-
Notifications
You must be signed in to change notification settings - Fork 32
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: Add enroll-by date to the assignment table #1248
Conversation
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.
Opted to move this function into the existing utils folder within the same directory.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1248 +/- ##
==========================================
+ Coverage 85.32% 85.35% +0.02%
==========================================
Files 538 539 +1
Lines 11860 11881 +21
Branches 2496 2535 +39
==========================================
+ Hits 10120 10141 +21
+ Misses 1682 1681 -1
- Partials 58 59 +1 ☔ View full report in Codecov by Sentry. |
src/components/learner-credit-management/AssignmentEnrollByDateCell.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/AssignmentEnrollByDateCell.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/AssignmentEnrollByDateCell.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/AssignmentEnrollByDateHeader.jsx
Outdated
Show resolved
Hide resolved
<Stack gap={0} direction="horizontal"> | ||
<span> | ||
<p data-testid="assignments-table-enroll-by-column-header" className="mb-0 mr-1"> |
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.
This is putting a block element (p
) in an inline element (span
), which is not valid HTML. For example, from this w3schools article about block vs. inline elements:
It's also not really using Stack
as intended since you have gap={0}
and are relying on the mr-1
class name to add the spacing between the text and the icon button (i.e., Stack
intends to remove the need for using spacing utility classes to implement gaps between elements).
I would suggest changing this to be more inline with the following Paragon Playground example.
<Stack gap={1} direction="horizontal">
<span data-testid="assignments-table-enroll-by-column-header">
Enroll-by date
</span>
<IconButtonWithTooltip
tooltipContent="Failure to enroll by midnight of enrollment deadline date will release funds back into the budget"
tooltipPlacement="right"
src={InfoOutline}
iconAs={Icon}
size="inline"
data-testid="enroll-by-date-tooltip"
/>
</Stack>
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.
Updated 👍🏽
There are currently 2 other components that follow the original pattern I implemented against. I am wondering if its worth updating those as well? It would not be considered a big lift.
MemberStatustableColumnHeader
MemberEnrollmentsTableColumnHeader
src/components/learner-credit-management/BudgetDetailPageOverviewAvailability.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/tests/BudgetDetailPage.test.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/AssignmentEnrollByDateCell.jsx
Outdated
Show resolved
Hide resolved
defaultMessage: 'When your plan expires you will lose access to administrative functions and the remaining balance of your plan{apostrophe}s budget(s) will be unusable. Contact support today to renew your plan.', | ||
description: 'Message for the notification that the Learner Credit plan is expiring in less than 30 days.', | ||
}, { aposrophe: "'" }), | ||
}, { apostrophe: "'" }), |
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.
[inform] I wonder if it's worth changing this up a bit to not treat apostrophe
as a dynamic property passed into the message, given it is static.
For example, would this work?
{
defaultMessage: "When your plan expires you will lose access to administrative functions and the remaining balance of your plan's budget(s) will be unusable. Contact support today to renew your plan.",
}
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.
LGTM! Just a handful of nits on those i18n strings using a dynamic message arg for the double quote "
character.
Adds a net new column to the learner credit management budget detail page on the assignments table called "Enroll-by date".
The column component determines if the date should render a "Warning" icon if the enroll by date is within 10 days of expiration.
Screen.Recording.2024-06-06.at.12.32.07.PM.mov
For all changes
Only if submitting a visual change