UI updates/ Launch Lessons page #12647
Conversation
There was a problem hiding this comment.
The CSS here looks great and the React clean-up and tests are much appreciated. I left some comments in places where I think we could take it a little further or make small changes to get us in line with best practices, but no need to review again. This page really needed a facelift and looks great now!
| font-weight: 600; | ||
| } | ||
| a { | ||
| color: #027360; |
There was a problem hiding this comment.
Maybe we could substitute this for a color variable?
There was a problem hiding this comment.
good catch! with the refactoring, I was in a bit of autopilot for some parts of copy and pasting 😅
| font-size: 14px; | ||
| font-weight: 400; |
There was a problem hiding this comment.
I think this could be replaced with @include text-s.
| @@ -19,6 +20,14 @@ export const DataTableChip = ({ color, icon, label, link }: DataTableChipProps) | |||
| </a> | |||
| ) | |||
| } | |||
| if (onClick) { | |||
| return ( | |||
| <button className={`data-table-chip ${color} focus-on-light`} onClick={onClick}> | |||
There was a problem hiding this comment.
Probably need a button type declaration here for the linter, right?
| return ( | ||
| <button className={`data-table-chip ${color} focus-on-light`} onClick={onClick}> | ||
| {icon && <img alt={icon.alt} src={icon.src} />} | ||
| <p>{label}</p> |
There was a problem hiding this comment.
I think a span is the more appropriate semantic element here -- I've been getting a lot of console errors about nesting block elements inside inline elements, which I think is technically not allowed under the HTML5 specification. There are some cases where it's tricky to avoid (like nesting elements in tables), but I think we should try where we can.
There was a problem hiding this comment.
ok! I'll make that change. Thank you for the context
| it('renders the unit name and assigned information', () => { | ||
| render(<Unit data={mockData} />); | ||
|
|
||
| expect(screen.getByText('Sample Unit')).toBeInTheDocument(); |
There was a problem hiding this comment.
This is something @anathomical often calls out, but I think that where something is a reference to a variable (as opposed to static text on the page), we should try to refer to it that way instead of just as a string, so it's clear what it should be testing.
There was a problem hiding this comment.
so something like this?
const unitNameSpan = screen.getByText('Sample Unit')
expect(screen.getByText(unitNameSpan)).toBeInTheDocument();
| if (Object.keys(classroomActivities)) { | ||
| return classroomActivities | ||
| } | ||
| if (Object.keys(classroom_activities)) { | ||
| return classroom_activities | ||
| } |
There was a problem hiding this comment.
Oy vey. My guess is you tried, but is it worth trying to track down why these are both sometimes present? Ideally, we'd only have one.
There was a problem hiding this comment.
I think it has something to do with the DB values for this data sometimes having different shapes (it's lessons so I didn't want to dive too deep into that hole)
| function getActivityId(lessonActivity) { | ||
| const { activityId, activityUid, activity } = lessonActivity | ||
| return activityId || activityUid || activity?.uid | ||
| } | ||
|
|
||
| function getClassroomUnitId(lessonActivity) { | ||
| const { cuId, classroom_unit_id } = lessonActivity | ||
| return cuId || classroom_unit_id | ||
| } |
There was a problem hiding this comment.
Same here. If it's just too complicated to deal with for this project that's fine, but might be worth noting in the PR description.
|
|
||
| function renderCustomizeCell(id) { | ||
| return( | ||
| <a className="customize-link focus-on-light" href={`/customize/${id}`} rel="noopener noreferrer" target="_blank"> |
There was a problem hiding this comment.
We should add an aria-label to these tags, because it won't be clear to a screenreader user what clicking that icon will do. Then, I'd test for the presence of an element with that aria label in the specs, instead of the alt text, which is less relevant for functionality.
There was a problem hiding this comment.
I'll add, thanks for the context!
|
|
||
| function renderDownloadCell(id) { | ||
| return( | ||
| <a className="download-link focus-on-light" href={`/activities/${id}/supporting_info`} rel="noopener noreferrer" target="_blank"> |
There was a problem hiding this comment.
See note above about aria labels.
| const rows = []; | ||
| const activities = getClassroomActivities() | ||
| activities.forEach((lessonActivity) => { |
There was a problem hiding this comment.
You could clean this up a little by doing const rows = getClassroomActivities().map(lessonActivity => { instead of pushing to the empty rows array.
There was a problem hiding this comment.
so this data actually comes through as a Map, so we have to use forEach to iterate through it and then push that relevant data into the array (I added a comment in the code for better context)
WHAT
design overhaul for the Launch Lessons page
WHY
this is a very old page and was due for a UI makeover
HOW
a lot of React and CSS refactoring
Screenshots
(If applicable. Also, please censor any sensitive data)
Notion Card Links
(Please provide links to any relevant Notion card(s) relevant to this PR.)
https://www.notion.so/quill/Update-UI-Launch-Lessons-ba7388f4d749439ab8c1ce22c09a5dc2
What have you done to QA this feature?
checked on staging that lessons data render as expected and various states are represented as expected