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

Changes alerts close btn to improve accessibility #23673

Merged
merged 2 commits into from Oct 1, 2017

Conversation

andresgalante
Copy link
Collaborator

@andresgalante andresgalante commented Aug 25, 2017

Changes alerts close btn position on the markup to improve accessibility

This PR is a simple order change of the close btn on alerts that improves the way a screen reader reads them.

Instead of going:

"Close, Holy guacamole! You should check in on some of those fields below, alert, Close, button"

now it goes:

"Holy guacamole! You should check in on some of those fields below, Close, alert, Close, button"

Which IMO it makes more sense.

Plus:

  • If the notification has an a a screen reader will tab order first the link then the close btn
  • this solution is more flexible since the user can put the btn markup anywhere and it'll render always on the top right corner disregarding the amount of text that the alert has.

Fixes #21889.

@patrickhlauke
Copy link
Member

This also looks good to me, leaving it up to @mdo for final say though.

@mdo
Copy link
Member

mdo commented Oct 1, 2017

It's unlikely to happen, but as it stands, someone could run into an issue where the dismiss icon overlays the text of the alert.

screen shot 2017-09-30 at 11 20 08 pm

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

This and the current version are both screwy, so merging away to at least solve one problem :D.

@mdo mdo merged commit f02f545 into twbs:v4-dev Oct 1, 2017
@mdo mdo mentioned this pull request Oct 1, 2017
@andresgalante
Copy link
Collaborator Author

@mdo I was aware of that problem, I'll try to think of a better solution. Thanks for reviewing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants