Skip to content
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

Merged
merged 3 commits into from Jun 4, 2020

Conversation

rohit2sharma95
Copy link
Collaborator

Closes #30773
Updated documentation and unit tests as well, for the modal component

- Update documentation
- Update unit tests
@rohit2sharma95 rohit2sharma95 requested a review from a team as a code owner May 31, 2020 09:23
Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@XhmikosR
Copy link
Member

XhmikosR commented May 31, 2020

@patrickhlauke should we backport it?

Not sure how easy that will be, just wondering.

@XhmikosR XhmikosR self-requested a review May 31, 2020 10:26
@patrickhlauke
Copy link
Member

we could backport it yes. it's more of a refinement/nice to have - saves authors having to manually add role="dialog" themselves, but even if they did/have it in their current code, it will still work

@XhmikosR
Copy link
Member

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.

@XhmikosR XhmikosR added this to Inbox in v5 via automation Jun 4, 2020
@XhmikosR XhmikosR added this to Inbox in v4.5.1 via automation Jun 4, 2020
@XhmikosR XhmikosR changed the title Add role="dialog" in the modal via script Add role="dialog" in modals via JavaScript Jun 4, 2020
Update the docs to mention that role="dialog" is already added via JavaScript
@XhmikosR XhmikosR merged commit 5faf41e into twbs:master Jun 4, 2020
v5 automation moved this from Inbox to Shipped Jun 4, 2020
@XhmikosR XhmikosR moved this from Inbox to Needs manual backport in v4.5.1 Jun 4, 2020
@XhmikosR XhmikosR moved this from Needs manual backport to Cherry picked in v4.5.1 Jun 4, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Jun 4, 2020

Now that I think about it, maybe we should not remove the role dialog on hide? It seems redundant.

/CC @Johann-S @patrickhlauke

@patrickhlauke
Copy link
Member

patrickhlauke commented Jun 4, 2020

doesn't really make a difference, since it's aria-hidden/display:none on hide. personally i like the idea that the script cleans up after itself, rather than modify the markup on first run and then leaving attributes hanging around that it injected...but it's probably more a philosophical matter and wouldn't be opposed to just leaving it there either.

@XhmikosR
Copy link
Member

XhmikosR commented Jun 4, 2020

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 :)

XhmikosR added a commit that referenced this pull request Jun 4, 2020
Add role="dialog" in modals via JavaScript
@rohit2sharma95 rohit2sharma95 deleted the modal-role-via-script branch June 4, 2020 14:29
XhmikosR added a commit that referenced this pull request Jun 4, 2020
Add role="dialog" in modals via JavaScript
@XhmikosR XhmikosR moved this from Cherry picked to Shipped in v4.5.1 Jun 4, 2020
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.5.1
  
Shipped
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

Add role="dialog" in the modal via script
3 participants