Skip to content

[added] Add contentLabel prop to put aria-label on modal content #243

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

Merged
merged 1 commit into from
Oct 15, 2016

Conversation

claydiffrient
Copy link
Contributor

@claydiffrient claydiffrient commented Oct 8, 2016

Fixes #236.

Changes proposed:

  • Add a aria-label to the modal content

Upgrade Path (for changed or removed APIs):

  • Existing modals should add a contentLabel prop describing the modal content

Acceptance Checklist:

  • All commits have been squashed to one.
  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

This makes the modal better follow aria guidelines for dialogs.
It also gives screenreader users some identity for the modal after
it opens.

closes #236
@claydiffrient
Copy link
Contributor Author

@diasbruno / @afercia would you mind reviewing this?

@diasbruno
Copy link
Collaborator

@claydiffrient Looks good! Ship it!

@diasbruno
Copy link
Collaborator

hmmmm, #230 would let people to choose what they want for data and aria attributes.

@claydiffrient
Copy link
Contributor Author

@diasbruno It doesn't look like #230 support all props just custom props. I think this is acceptable to make sure that the aria-label can be applied. I think once the merge conflicts are resolved on #230 then we can merge it as well.

@claydiffrient claydiffrient merged commit 3d8e5a0 into master Oct 15, 2016
@claydiffrient claydiffrient deleted the feature/a11y/label branch October 15, 2016 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants