-
Notifications
You must be signed in to change notification settings - Fork 515
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
feat(macros): add Accessibility sidebar #10659
Conversation
Co-authored-by: Onkar Ruikar <87750369+OnkarRuikar@users.noreply.github.com>
Co-authored-by: Onkar Ruikar <87750369+OnkarRuikar@users.noreply.github.com>
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.
Great initiative to add a proper sidebar macro for the accessibility docs! 🙌
Please note that we prefer a more declarative approach for sidebars these days, which declares each section as a list of slugs. See:
Other than that I added some specific suggestions towards a more declarative sidebar.
kumascript/macros/A11yRef.ejs
Outdated
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 think the "*Ref" name pattern has always been a bit non-obvious. Can we rename it to A11ySidebar
or (even better, since we don't have to save space) AccessibilitySidebar
?
Co-authored-by: Claas Augner <495429+caugner@users.noreply.github.com>
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.
LGTM overall, thanks a lot!
I left some comments, let me know what you think.
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.
Nit: Let's uppercase the "A" in "AccessibilitySidebar" for consistency with the other recent sidebar macros.
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.
Name changed and all suggested edits made.
mdn/content#32818 is dependent on this PR being merged |
Hi @caugner. This is ready for a re-review and, hopefully, a merge. Thanks. |
|
Thank you @estelle, great job! 🙌 |
Summary
New sidebar for the accessibility section
Problem
There is no sidebar for accessibility. Instead, each page has a variation of:
Solution
This is a new sidebar for that section. English only, but localization is possible.
Screenshots
Before
After
How did you test this change?
local build.
new page.