refactor: migrate off paragon modal deprecated migration #872
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #872 +/- ##
==========================================
- Coverage 85.43% 83.02% -2.41%
==========================================
Files 497 412 -85
Lines 10777 8830 -1947
Branches 2262 1816 -446
==========================================
- Hits 9207 7331 -1876
+ Misses 1527 1460 -67
+ Partials 43 39 -4 ☔ View full report in Codecov by Sentry. |
…n-portal into mashal-m/paragon-modal-deprecation
|
@adamstankiewicz, not sure who to direct the review request for this one, but something tells me you might know. |
| /> | ||
| <div ref={this.modalRef}> | ||
| <ModalDialog | ||
| isOpen |
There was a problem hiding this comment.
nit: technically, we should be rendering this component with isOpen={false} instead of only rendering CodeAssignmentModal itself when isOpen=true already. I defer to you if you'd like to tackle this refactoring through this PR as this is an existing problem and a bit out of scope of just moving Modal -> ModalDialog here.
There was a problem hiding this comment.
I get it, I guess there are already many changes in this PR, a separate PR for this work would be better.
| > | ||
| <> | ||
| {mode === MODAL_TYPES.assign && submitting && <Icon className="fa fa-spinner fa-spin mr-2" />} | ||
| {`Assign ${isBulkAssign ? 'Codes' : 'Code'}`} |
There was a problem hiding this comment.
nit: the convention for our CTA buttons is to have sentence case (e.g., "Assign codes", "Assign code", "Save template")
| <ModalDialog.CloseButton variant="link"> | ||
| Cancel | ||
| </ModalDialog.CloseButton> | ||
| <Button |
There was a problem hiding this comment.
nit: This Button could be replaced with a StatefulButton (docs), but may be out of scope of this PR.
There was a problem hiding this comment.
Right, there are already many changes in this PR, a separate PR for this work would be better.
Mashal-m
force-pushed
the
mashal-m/paragon-modal-deprecation
branch
from
3084981 to
e89f772
Compare
…n-portal into mashal-m/paragon-modal-deprecation
Mashal-m
force-pushed
the
mashal-m/paragon-modal-deprecation
branch
from
1138dd7 to
473c88d
Compare
…n-portal into mashal-m/paragon-modal-deprecation
|
This has stagnated for 6 months, given that the only issues are |
|
@feanil Just a heads up... for whatever reason this repo does not seem to be failing CI on PRs when linting throws errors. If you look at the "Files changed" on this PR, you'll see several linting annotations, one of which is throwing several JS errors in production, causing a critical user flow to break where our admins can't invite learners to their subscription plans (i.e., We will fix forward. Also worth noting, the Enterprise Quokkas team did recently pick up an epic to wrap up the Paragon deprecation work in this MFE; Enterprise Engineering was purposely holding off on reviewing/fixing/merging the Paragon deprecation PRs in this repo until we could dedicate adequate time for QA, etc., especially with the |
|
Hey @adamstankiewicz I'm sorry that this caused issues in production. Good to know that further work for the deprecation is getting prioritized now. Is there a github issue or ticket on your side for fixing CI so that we can catch these issues before it goes to production. Also, are there automated tests that would have caught this outside of the linting that weren't run or need to be written? |
Ticket
Migrate off deprecated Paragon components
What has Changed?
Migrate off paragon modal deprecation in