-
Notifications
You must be signed in to change notification settings - Fork 3
enrollment dashboard mobile layout #2187
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
Conversation
OpenAPI ChangesShow/hide No detectable change.
|
cf4eca8
to
b987055
Compare
OpenAPI ChangesShow/hide No detectable change.
|
1 similar comment
OpenAPI ChangesShow/hide No detectable change.
|
OpenAPI ChangesShow/hide No detectable change.
|
1 similar comment
OpenAPI ChangesShow/hide No detectable change.
|
d96dcdf
to
b503241
Compare
OpenAPI ChangesShow/hide No detectable change.
|
1 similar comment
OpenAPI ChangesShow/hide No detectable change.
|
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 great. One minor suggestion, and one request about tests.
</Stack> | ||
{run.startDate ? ( | ||
<CourseStartCountdown startDate={run.startDate} /> | ||
) : null} | ||
</Stack> | ||
</CardRoot> | ||
) | ||
|
||
const mobileLayout = ( | ||
<CardRoot data-testid="enrollment-card"> |
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.
Request: I think we should use separate testids for the desktop/mobile versions, and one of them (doesn't matter which) should just be enrolment-card
. (Or I guess maybe we should change it to dashboard-card
).
The current structure has a few disadvantages, imo:
- makes the
DashboardCard.test.tsx
file awkward. You had change a bunch of things likecourseLink
tocourseLinks
, and it's not clear how manycourseLinks
there are. - The tests for a component that renders a list of dashboard cards now feels weird, too. If the test renders 7 resources, you get 14 cards.
If we use separate testids, then both these problems go away:
- the
DashboardCard.test.tsx
can make all the assertions it wants about both variations,and it's clear there is only onecourseLink
(per variation). - Other tests see 1
enrollment-card
per resource. (And also 1enrollment-card-mobile
per resource, but other tests can ignore that...the equivalence of desktop/mobile versions in terms of logic / meaning is thoroughly tested inDashboard.test.tsx
I took a quick stab at this in https://github.com/mitodl/mit-learn/compare/cg/enrollment-dashboard-mobile...cg/enrollment-dashboard-mobile-cc-edits?expand=1 (The DashboardCard.test.tsx file passes; some other test file fails because it is expecting 2 cards per resource. I didn't change that one.)
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.
If you look at https://github.com/mitodl/mit-learn/compare/cg/enrollment-dashboard-mobile...cg/enrollment-dashboard-mobile-cc-edits?expand=1 you can see my old man bias against mobile... Could have called them enrollment-card
and enrollment-card-desktop
😛
width="100%" | ||
> | ||
<Stack direction="column" gap="8px"> | ||
<Link size="medium" color="black" href={marketingUrl}> |
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.
OpenAPI ChangesShow/hide No detectable change.
|
1 similar comment
OpenAPI ChangesShow/hide No detectable change.
|
@ChristopherChudzicki Thanks, this is ready for another look. |
const upgradeRootDesktop = screen.queryByTestId("upgrade-root-desktop") | ||
const upgradeRootMobile = screen.queryByTestId("upgrade-root-mobile") |
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.
My preference would be to reduce the number of places we append desktop and mobile and use within
instead:
const upgradeRootDesktop = within(scree.getByTestId("enrollment-card-desktop")).queryByTestId("upgrade-root")
const upgradeRootMobile = within(scree.getByTestId("enrollment-card-mobile")).queryByTestId("upgrade-root")
93f1b24
to
81f2ae6
Compare
OpenAPI ChangesShow/hide No detectable change.
|
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.
👍
OpenAPI ChangesShow/hide No detectable change.
|
What are the relevant tickets?
https://github.com/mitodl/hq/issues/7042
Description (What does it do?)
This PR implements mobile designs on the enrollments display in the dashboard home page.
Screenshots (if appropriate):
How can this be tested?
mit-learn
enrollment-dashboard
feature flag