-
Notifications
You must be signed in to change notification settings - Fork 0
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
Switch alert show/hide to a prop #581
Conversation
@echappen Just to check, should I be seeing messages/alerts in (If I should be testing somewhere else, just let me know.) |
@beepdotgov Yep, Alerts are now wrapped in aria-live regions (which are now present on page-load). So whatever text is in the alert should be the text read by the SR. You can trigger success alerts by updating roles for any user on the Manage Users page. |
This ensures that the aria-live region in the Alert component is present on page load.
01d48af
to
6dda1f7
Compare
Ach, sorry — must’ve had something cached. I see the |
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.
Just want to say that I was able to update some permissions, and the success notification was successfully read out each time! Exciting to see this fixed! 🎉
@beepdotgov Thank you for all your manual testing and collaboration on this! |
Thank you for doing such thoughtful research on this, @echappen. I’ve learned a lot watching you work through this issue! |
Changes proposed in this pull request:
Alert
show-hiding to a prop (this ensures that the aria-live region in the Alert component is present on page load)Related issues
Closes #531
Overrides #573
Submitter checklist
Security considerations
None, UI refactor only