-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
refactor(v2): improve semantic doc sidebar markup #4940
Conversation
)} | ||
aria-label={translate({ | ||
id: 'theme.docs.sidebar.menuLabel', | ||
message: 'Docs navigation', |
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.
I just thought that maybe we shouldn't use the word "docs" in ARIA labels? After all, it won't always be exactly docs, right? Maybe should use "Secondary navigation" or something like that?
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.
Gatsby has "Secondary navigation" so why not. Isn't there value to also mention it's a sidebar? like Secondary navigation (sidebar)
or something?
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.
Propose simply "Sidebar navigation".
✔️ [V1] 🔨 Explore the source changes: eae62d8 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-1/deploys/60c7cb0c7edd1400073a4011 😎 Browse the preview: https://deploy-preview-4940--docusaurus-1.netlify.app |
Size Change: +753 B (0%) Total Size: 621 kB
ℹ️ View Unchanged
|
✔️ [V2] 🔨 Explore the source changes: eae62d8 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60c7cb0c3542220007d90330 😎 Browse the preview: https://deploy-preview-4940--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4940--docusaurus-2.netlify.app/ |
)}> | ||
)} | ||
aria-label={translate({ | ||
id: 'theme.docs.sidebar.menuLabel', |
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.
id: 'theme.docs.sidebar.menuLabel', | |
id: 'theme.docs.sidebar.menuAriaLabel', |
By convention, making it clear it's an aria label through the id is helpful for translators to understand why they don't find this string on the UI (not all will try to open base.json to see the description)
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.
what about navAriaLabel
for consistency with the blog?
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.
Done
)} | ||
aria-label={translate({ | ||
id: 'theme.docs.sidebar.menuLabel', | ||
message: 'Docs navigation', |
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.
Gatsby has "Secondary navigation" so why not. Isn't there value to also mention it's a sidebar? like Secondary navigation (sidebar)
or something?
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.
I'd prefer if all aria label keys contain AriaLabel
by convention and for consistency
👍 |
Motivation
Just make the markup for the doc sidebar a little more semantic.
Refs:
For example, Bootstrap, VuePress, Next.js sites use similar markup.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Preview (no visual changes, actually)
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)