-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Add role="dialog" in modals via JavaScript #30936
Conversation
- Update documentation - Update unit tests
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
@patrickhlauke should we backport it? Not sure how easy that will be, just wondering. |
we could backport it yes. it's more of a refinement/nice to have - saves authors having to manually add |
Ok, thanks! I'll think about backporting it, but it does seem this is a non-breaking change, i.e. if the attribute already exists, it will be updated. |
Update the docs to mention that role="dialog" is already added via JavaScript
Now that I think about it, maybe we should not remove the role dialog on hide? It seems redundant. |
doesn't really make a difference, since it's |
Yeah, it may be better. I was just thinking that it's also better the lest stuff we do with the DOM, the better, that's all :) |
Closes #30773
Updated documentation and unit tests as well, for the modal component