-
Notifications
You must be signed in to change notification settings - Fork 303
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
Simplify guidance in Banner a11y #789
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.
Definitely a lot cleaner and more straightforward! Just some minor comments/clarification requests.
content/components/banner.mdx
Outdated
* **Live region announcements**: can be used to announce the new content to screen reader users. This method may be the preferred method for non-critical information (**with exceptions**). | ||
* **Focus management**: involves shifting a user's focus directly the new Banner. This method is disruptive when used inappropriately, but is extremely helpful in scenarios where we need to guide the user towards an action (**with exceptions**). |
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.
A link to the exceptions or an indication that these exceptions will be mentioned later in the doc would be helpful.
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 added a bit more detail around what I mean by exception for the first bullet point.
I removed mention of "with exceptions" for the latter point since I couldn't think of any/ it didn't make sense.
content/components/banner.mdx
Outdated
|
||
#### Is there already a live region on the page that can be utilized? | ||
If a user triggers an action that results in the element that they were were on being removed from the DOM, then focus management is required. This is mostly relevant in SPA/React where elements in the DOM are replaced, resulting in focus loss. |
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.
This paragraph sounds like it's covering where focus is placed when the banner is dismissed. It may not be, but it's a bit vague. We might need an example or a tie back into the section title 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.
Thanks Chelsea! I added an example, hope it helps!
content/components/banner.mdx
Outdated
|
||
Please reach out to the accessibility team if you need help determining the best approach for your use case! | ||
If the Banner appears as part of the new content on the page after a user experiences focus loss, then it may be an appropriate focus target. |
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.
We should clarify but not when the page content changes or we go to a brand new page
Co-authored-by: Chelsea Adelman <40274682+ichelsea@users.noreply.github.com>
Co-authored-by: Chelsea Adelman <40274682+ichelsea@users.noreply.github.com>
@ichelsea thank you for your feedback! I did my best to incorporate them. 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.
Love it! Great work
This PR simplifies the guidance in Banner A11y to emphasize the key points, and provide some more nuance around when to prefer live region or focus management.