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

Fix duplicate navigation for lesson nav #2806

Merged
merged 7 commits into from
Aug 1, 2024

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Jul 31, 2024

Fixes #2782

This PR removes the entire screen-reader-text div - ie. the nav & the first "completed" button are removed.
Also added a visible "Previous Lesson" button to sensei lesson actions.
And since the style of the Sensei next lesson button cannot align well with the previous lesson button on smaller screens (either it doesn't automatically become two rows or it becomes unclickable), a new "Next Lesson" button is also added.

I haven't figured out how to indicate that the current lesson is already completed. Initially it was a button-like div, but it caused confusion, so I removed it. Any thoughts on this? @WordPress/meta-design
image

Screencasts

Before

Before.mov

After

After.mp4

Smaller screen
different screen size

@renintw renintw requested a review from ryelle July 31, 2024 22:13
@renintw renintw self-assigned this Jul 31, 2024
@renintw renintw added [Type] Bug Something isn't working on the Learn website. [Component] Sensei Website development issues related to the Sensei plugin installed on Learn. Accessibility Fix or enhancement related to accessibility. labels Jul 31, 2024
@renintw renintw added this to the Learning Pathways launch milestone Jul 31, 2024
@jasmussen
Copy link

The progressbar is the main piece.

Changing the text of the button, and making it disabled, is a good idea. Could look like this:
light gray

Noting here that the button is disabled, and disabled elements do not need to meet contrast requirements, and therefore it is intentionally very light. We can also darken that text to charcoal 5:

charcoal 5

@renintw renintw force-pushed the 2782-fix-duplicate-navigation-for-lesson-nav branch from 8925326 to 4d465ec Compare August 1, 2024 13:51
@renintw
Copy link
Contributor Author

renintw commented Aug 1, 2024

Thanks @jasmussen. Does this match your expectations of the result?

lesson.complete.mp4

@jasmussen
Copy link

Yes it does. There may still be optimizations to do with regards to the next/previous lesson buttons and where they sit, perhaps they are reduced to just "Next" and "Previous", and are arrows instead of the bigger buttons. But we're getting into a detail territory that isn't necessarily a blocker for this particular PR.

@ryelle
Copy link
Contributor

ryelle commented Aug 1, 2024

Changing the text of the button, and making it disabled, is a good idea.
Noting here that the button is disabled, and disabled elements do not need to meet contrast requirements, and therefore it is intentionally very light.

I disagree with this. Part of the original feedback in the a11y issue was that "the second button is an interactive control that is not disabled, but does not do anything. This is confusing for all users. It should just be text, and not be an interactive element."

It is not really a disabled button, it's meant to convey information to the user. There's a suggestion that we could add the "completed" label to the main lesson title, and I think that would be better - maybe something like this

Screenshot 2024-08-01 at 11 41 10 AM

Then we could remove the "Completed" faux-button from the end of the lesson, so it only has the next/previous buttons.

@@ -0,0 +1,14 @@
function init() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be done with CSS — display:none hides content from everyone, including screen readers.

.sensei-lesson-footer .screen-reader-text {
    display: none;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah, I was initially thinking about completely removing it, so I went ahead and did it that way. Thanks!
2899198

@renintw
Copy link
Contributor Author

renintw commented Aug 1, 2024

It is not really a disabled button, it's meant to convey information to the user. There's a suggestion that we could add the "completed" label to the main lesson title, and I think that would be better - maybe something like this

Yeah, actually this is what I had in mind originally, but I also feel that using a disabled button could be acceptable, maybe just treating it as a button-like div, as with that commit, the screen reader wouldn't read it as a button, it would just read the text "lesson completed." But it might be a bit confusing to sighted users and adding a label is definitely better.

@renintw
Copy link
Contributor Author

renintw commented Aug 1, 2024

maybe something like this
image

I'll try this first. However, I was originally thinking about adding a green label somewhere to make it more obvious, for example like adding an icon like you did and making the entire title green. Also, the "completed" status icon in the sidebar should probably be green as well, to make it more consistent with other places. This can be done post-launch, though.

Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Looks good! I tested with VoiceOver and no extra buttons/links exist. I think the title update can be handled in a future PR. Good to merge pending the unused CSS removal.

Comment on lines 54 to 59

.disabled {
color: var(--wp--preset--color--charcoal-5) !important;
border-color: var(--wp--preset--color--charcoal-5) !important;
pointer-events: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove this style, since the disabled button has been removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryelle
Copy link
Contributor

ryelle commented Aug 1, 2024

However, I was originally thinking about adding a green label somewhere to make it more obvious, for example like adding an icon like you did and making the entire title green. Also, the "completed" status icon in the sidebar should probably be green as well, to make it more consistent with other places. This can be done post-launch, though.

This was definitely thrown together in dev tools for a suggestion. I think this can wait until post-launch so we can get some proper design input for it.

@renintw
Copy link
Contributor Author

renintw commented Aug 1, 2024

This was definitely thrown together in dev tools for a suggestion. I think this can wait until post-launch so we can get some proper design input for it.

Opened an issue for this.

@renintw renintw merged commit 6f967f4 into trunk Aug 1, 2024
1 check passed
@renintw renintw deleted the 2782-fix-duplicate-navigation-for-lesson-nav branch August 1, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Fix or enhancement related to accessibility. [Component] Sensei Website development issues related to the Sensei plugin installed on Learn. [Type] Bug Something isn't working on the Learn website.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

A11y / Sensei / Learn - Duplicate navigation for lesson nav
3 participants