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(AlertGroup): Make dynamic alerts more accessible #5946
Conversation
PF4 preview: https://patternfly-react-pr-5946.surge.sh |
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'm for this change. I think it draws attention to a critical aspect of the overall AX and provides a nice hook for ensuring Alerts are announced consistently.
The only thing I wonder is if the extra API part is needed at all, and it's not just a documentation/illustration exercise. For example, setting isDynamic
is just like typing aria-live="polite"
. Are we missing an opportunity to expose developers to the aria API by doing it for them? Just asking that question for conversation's sake, I think we should use it but just rename it to be more declarative.
What's missing may be an explanation in the documentation page about how it's required for the live region container to exist on the page prior to the content being changed for assistive technology to "notice" the update which in turn is what allows the changes to be announced. For example..
Under "Static alert group" example heading, maybe mention
These alerts are discoverable from within the normal page content flow, and will not be announced individually/explicitly to assistive technology.
Under "Toast alert group" example heading(or whichever example uses isDynamic/isLiveRegion), maybe mention something like
Alerts asynchronously appended into dynamic AlertGroups will be announced to assistive technology at the moment the change happens, following the strategy used for aria-atomic, which defaults to false, which means only changes of type "addition" will be announced. Of course, this is dependent on the user agent in question.
packages/react-core/src/components/AlertGroup/examples/AlertGroup.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/AlertGroup/AlertGroupInline.tsx
Outdated
Show resolved
Hide resolved
This is great! Should we update the Dynamic alert examples as part of this effort, too? |
@jessiehuff should we make a follow up issue to update Dynamic alerts examples and dynamic helper text examples? |
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.
These changes are great! I have one small recommendation on how to make the Alert/AlertGroup async examples more helpful. Hope it helps!
5011781
to
0df21a9
Compare
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.
Nice work!
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Alternative approach/attempt to PR#5773
Closes #5731
This PR/POC is meant to test an alternative approach to making dynamic alerts more accessible. It turns AlertGroup into a pre-existing live region for screen readers to be notified that alerts will be rendered inside of it.