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.

Sign up for GitHub

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

Jump to bottom

fix(theme): improve docs sidebar category caret aria-label accessibility #9269

Merged
merged 4 commits into from
Sep 14, 2023
Merged

fix(theme): improve docs sidebar category caret aria-label accessibility #9269

merged 4 commits into from
Sep 14, 2023

Conversation

pinakipb2
Copy link
Contributor

@pinakipb2 pinakipb2 commented Aug 28, 2023 •
edited by slorber
Loading

Pre-flight checklist

Motivation

Fixes #9162

Test Plan

Test links

https://deploy-preview-9269--docusaurus-2.netlify.app/

Related issues/PRs

#9162

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 28, 2023
@netlify
Copy link

netlify bot commented Aug 28, 2023 •
edited
Loading

✅ [V2]

Name Link
🔨 Latest commit 7c02820
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6502c71052fe8a000836f231
😎 Deploy Preview https://deploy-preview-9269--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions
Copy link

github-actions bot commented Aug 28, 2023 •
edited
Loading

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 74 🟢 97 🟢 92 🟢 100 🟠 89 Report
/docs/installation 🟠 79 🟢 98 🟢 92 🟢 100 🟠 89 Report

slorber
slorber requested changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@slorber slorber left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reading #9269, I'm not 100% sure it's what @msftedad want, but we'll see when they review. In any case that seems like a valid aria-label an improvement that we'll want to merge.

I'd prefer if you didn't duplicate the button JSX because only 1 prop is different for the 2 cases

packages/docusaurus-theme-classic/src/theme/DocSidebarItem/Category/index.tsx Outdated Show resolved Hide resolved
@slorber
Copy link
Collaborator

slorber commented Aug 31, 2023

@msftedad can you please review this PR and tell us if it improves accessibility for you?

Preview url: https://deploy-preview-9269--docusaurus-2.netlify.app/

Thanks

@slorber slorber changed the title feat(a11y): Fix a11y in sidebar fix(a11y): fix docs sidebar category caret button accessibility, improve aria-label Aug 31, 2023
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Aug 31, 2023
@slorber slorber mentioned this pull request Aug 31, 2023
Closed
7 tasks
@pinakipb2 pinakipb2 requested a review from slorber August 31, 2023 10:22
@pinakipb2
Copy link
Contributor Author

pinakipb2 commented Sep 1, 2023 •
edited
Loading

Hi @slorber its not "fix(a11y): fix docs sidebar category caret button accessibility, improve aria-label" as its not only for docs, it will affect the sidebar on every page using docusaurus. Its a change on the main product. The sidebar of the generated sites will have this a11y. Its better for users with screen readers to know if the sidebar category button is expand/collapse type so that they know the action they are performing.

Ignore if you mean the documentation site(site generated by docusaurus) as docs.

@pinakipb2 pinakipb2 changed the title fix(a11y): fix docs sidebar category caret button accessibility, improve aria-label fix(a11y): fix sidebar category caret button accessibility, improve aria-label Sep 1, 2023
@pinakipb2 pinakipb2 changed the title fix(a11y): fix sidebar category caret button accessibility, improve aria-label fix(a11y): fix docs sidebar category caret button accessibility, improve aria-label Sep 1, 2023
@slorber
Copy link
Collaborator

slorber commented Sep 1, 2023

Hi @slorber its not "fix(a11y): fix docs sidebar category caret button accessibility, improve aria-label" as its not only for docs, it will affect the sidebar on every page using docusaurus. Its a change on the main product. The sidebar of the generated sites will have this a11y. Its better for users with screen readers to know if the sidebar category button is expand/collapse type so that they know the action they are performing.

Ignore if you mean the documentation site(site generated by docusaurus) as docs.

Sorry but I don't understand 😅

@pinakipb2
Copy link
Contributor Author

pinakipb2 commented Sep 1, 2023 •
edited
Loading

Sorry but I don't understand 😅

Hi @slorber, people who are visually impaired rely on screen readers, the screen readers read the content of the website to them. But some complex components like button with a icon, its really hard for them(screen readers) to know the function of the button, and they don't know what to say aloud. So aria-label is used to so that they know what the button does and they say that aloud. So, in sidebar, in dropdown now the screen reader will say if the dropdown is collapsed or expanded, so the visually impaired person can know if clicking it will collapse the menu or expand it. As docusaurus generates website, which has sidebar, so the now the newly created websites with docusaurus will have better a11y for such persons in the sidebar.

As previously it was reading "Toggle the collapsible sidebar category 'Getting Started'", so it was unclear now it will be reading "Expand/Collapse the collapsible sidebar category 'Getting Started" based on the button state.

image

Hope you understand it now. 😉

@Josh-Cena Josh-Cena changed the title fix(a11y): fix docs sidebar category caret button accessibility, improve aria-label fix(theme): docs sidebar category caret accessibility, improve aria-label Sep 5, 2023
@Josh-Cena Josh-Cena changed the title fix(theme): docs sidebar category caret accessibility, improve aria-label fix(theme): improve docs sidebar category caret aria-label accessibility Sep 5, 2023
@pinakipb2
Copy link
Contributor Author

@slorber @msftedad Have a look on the PR !

@pinakipb2
Copy link
Contributor Author

Hi @slorber, as mentioned in #9162 (comment) by @msftedad, you can now merge this PR.
Thanks !

slorber
slorber approved these changes Sep 14, 2023
View reviewed changes
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks 👍

Just renamed the "isActive" props to "collapsed" because it's not super clear what isActive means if we extract later the button to a separate file

@slorber slorber merged commit 83d9f22 into facebook:main Sep 14, 2023
@pinakipb2 pinakipb2 deleted the sidebar-a11y branch September 14, 2023 11:21
This was referenced Oct 19, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slorber slorber slorber approved these changes

@lex111 lex111 Awaiting requested review from lex111

@Josh-Cena Josh-Cena Awaiting requested review from Josh-Cena Josh-Cena is a code owner

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Milestone
No milestone
3 participants
@pinakipb2 @slorber @facebook-github-bot