-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: display warning when plan is about to expire #1177
feat: display warning when plan is about to expire #1177
Conversation
31c08d6
to
ba31b6a
Compare
db6d182
to
859753c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments/suggestions (most notably around removing the need to do as much prop drilling, etc. to pass budgets
related data into your BudgetExpiryComponent
), and several clarifying questions about the removal of the no longer relevant "Contact representative" modal, and some duplicative functions serving the same purpose.
Note, it'd also be great to include screenshots in the PR description for UI-facing changes such as these to aid reviewers as well (per the PR checklist template).
src/components/BudgetExpiryComponent/BudgetExpiryComponentWrapper.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/BudgetDetailPageHeader.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/BudgetDetailPageOverviewAvailability.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/BudgetDetailPageOverviewAvailability.jsx
Outdated
Show resolved
Hide resolved
d7ca0cd
to
5750f0d
Compare
8bd8853
to
f28856e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I just have one clarifying question about the changes in orderBudgets
that I might recommend looking into before this merges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempted to review but some unaddressed PR feedback remaining. Will check tomorrow morning for a quick lookover.
733a6c5
to
8bc645f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1177 +/- ##
==========================================
+ Coverage 85.46% 85.54% +0.07%
==========================================
Files 508 514 +6
Lines 11074 11169 +95
Branches 2329 2348 +19
==========================================
+ Hits 9464 9554 +90
- Misses 1566 1571 +5
Partials 44 44 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with 1 nit that needs to be addressed.
e86e9cb
to
e972937
Compare
e972937
to
eef9e65
Compare
<div className="mt-4"> | ||
<BudgetExpiryAlertAndModal /> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if no alert is rendered, it appears (per the Admin.test.jsx.snap snapshot file) this renders an empty div with mt-4
. Is this adding extra whitespace to the Learner Progress Report when there is no budget expiry alert?
Perhaps the .mt-4
could be rendered within the div.col
below where we render the <h2>
heading, which already has a .mt-4
specified all the time.
const thresholdKeys = Object.keys(ExpiryThresholds).sort((a, b) => a - b); | ||
const thresholdKey = thresholdKeys.find((key) => durationDiff.asDays() <= key); | ||
|
||
if (thresholdKey === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is the check against undefined
necessary or could it be simplified to !thresholdKey
?
export const isPlanApproachingExpiry = (endDateStr) => { | ||
const { thresholdKey, threshold } = getExpirationMetadata(endDateStr); | ||
|
||
if (thresholdKey === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is the check against null
necessary or could it be simplified to !thresholdKey
?
For all changes
Only if submitting a visual change
6a54ca9b-86fb-4d50-abc5-91f8fc1de163.mp4