-
Notifications
You must be signed in to change notification settings - Fork 2
Sidebar a11y fixes #59
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
Conversation
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.
Pull Request Overview
This PR improves sidebar accessibility by moving the sidebar before the main content and wrapping it in an aside element with a dynamic aria-label.
- Sidebar is now rendered within an aside element with a dynamic label based on the active header link or site title.
- The previous static sidebar pane has been removed to maintain a logical DOM order.
hidden={{narrow: true}} | ||
divider="line" | ||
> | ||
<aside aria-label={`${activeHeaderLink ? activeHeaderLink.title : siteTitle} sidebar`}> |
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.
PageLayout.Pane
is not polymorphic, so we need to nest the aside
region here.
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.
Works great 👍
Resolves https://github.com/github/primer/issues/5381
This pull request moves the sidebar before the content in the DOM and introduces an
aside
region to enhance accessibility. The aria-label now dynamically displays the appropriate title based on the active header link or site title.