Skip to content
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

Add SelfPacedProgressTable to My PL LandingPage component #57869

Merged
merged 10 commits into from Apr 10, 2024

Conversation

kelbyhawn
Copy link
Contributor

@kelbyhawn kelbyhawn commented Apr 8, 2024

Adds the SelfPacedProgressTable component to https://studio.code.org/my-professional-learning

  • This section only shows up when a teacher has Self-paced PL course activity
    • Updated the unit tests for this page and tested locally to make sure this works
  • Created a constants.js file to share test data between Storybook and landingPageTest.js unit tests (thanks for the tip @hannahbergam!)
  • Also added the tableStyles.scss stylesheet to this page so the My Workshops table matches the updated UI
    • This will get an overhaul soon too

Followup: the existing self-paced courses section on the Teacher homepage will be removed and replaced with a link to check this page to see self-paced course progress (Jira ticket)

Jira ticket: ACQ-1715


Has Self-Paced PL Courses

localhost-studio code org_3000_my-professional-learning

New Teacher (w/o Self-Paced courses)

localhost-studio code org_3000_my-professional-learning

@kelbyhawn kelbyhawn requested a review from a team as a code owner April 8, 2024 18:22
@@ -73,6 +75,7 @@ export default function LandingPage({
text: i18n.plLandingGettingStartedButton(),
},
]}
marginBottom={'0'}
Copy link
Contributor Author

@kelbyhawn kelbyhawn Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to remove the bottom margin from the Getting Started TwoColumnActionBlock component used on this page for new teachers.

}

.headerContainer {
background-color: color.$light_gray_50;
padding: 0 1rem;
Copy link
Contributor Author

@kelbyhawn kelbyhawn Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a little bit of refactoring here related to this PR since the inline padding was being applied to all the <section>s on the page. Here's a screenshot of the tabs showing to confirm this didn't break, but these will not show up on the page yet!

Screenshot 2024-04-09 at 11 03 02 AM

@@ -17,6 +17,7 @@ table.selfPacedProgressTable {
height: 0.5rem;
border-radius: 1rem;
margin-block-start: 0.25rem;
overflow: hidden;
Copy link
Contributor Author

@kelbyhawn kelbyhawn Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a little UI bug on the progress bar if it gets close to the end:

Before After
Before After

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That attention to detail🔥

@kelbyhawn kelbyhawn requested a review from a team April 9, 2024 18:09
Copy link
Contributor

@hannahbergam hannahbergam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! Looks great! One small comment but non-blocking

@@ -39,7 +39,7 @@ const CourseRow = ({
{percent_completed}% {i18n.selfPacedPlCompleted()}
</BodyThreeText>
{/* Progress bar */}
<div className={styles.progressBar}>
<div className={styles.progressBar} data-testid="progress-bar">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought for future/followup work- it would be super cool if this had some sore of accessible readout (i.e. label) of progress! I know that isn't a focus for this page but always something to keep in mind in case it's easy to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about adding something like that! But it says "(percent)% Completed" right above it so I thought it would be redundant 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oo does a screen reader read that out loud? I'm not sure if this is what you're referring to, but there is a way to add an aria-label that doesn't show, but a screen reader would still read

Copy link
Contributor Author

@kelbyhawn kelbyhawn Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh it doesn't! VoiceOver on Mac only reads links and buttons. I checked this on the Teacher homepage, and it does the same thing there too (only reads links and buttons). I'll add a ticket for this in the next AccHack.

Edit: made a ticket: https://codedotorg.atlassian.net/browse/A11Y-281

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay- it should be as simple as wrapping the text in a . But yes- thanks for making a ticket!

Copy link
Contributor Author

@kelbyhawn kelbyhawn Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither the paragraph or progress bar is a link, so it'd need to be something else. Misread that to say "wrap in an a (link tag)" 🤦‍♀️ I'm too scattered today to look into it, but will look tomorrow! This makes me excited for the Deque training, since I assumed screen readers read all the text on a page 🥲

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes okay I'm following! To be clear, folks using screen readers use the tab key to scan through interactive elements (like you mentioned- buttons, links, input fields, etc), but they can also use keys to navigate by header size (and drill down into reading the text directly if they choose). This is a super interesting case because A11y-wise, really we should be treating this div as an image with an alt text. Happy to brainstorm together at some point!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use keys to navigate by header size (and drill down into reading the text directly if they choose

A ha! That makes sense, thanks for explaining. This is an interesting case…maybe the <figure> and <figcaption> elements would work? (MDN docs)

Let's def brainstorm on this, and thanks for bringing it up!

@@ -17,6 +17,7 @@ table.selfPacedProgressTable {
height: 0.5rem;
border-radius: 1rem;
margin-block-start: 0.25rem;
overflow: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That attention to detail🔥

@kelbyhawn kelbyhawn merged commit 3da2ed4 into staging Apr 10, 2024
2 checks passed
@kelbyhawn kelbyhawn deleted the add-self-paced-section-to-my-pl-page branch April 10, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants