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

feat(modal): support alertdialog aria role #5955

Merged
merged 5 commits into from
Jul 22, 2020

Conversation

FrivalszkyP
Copy link
Contributor

@FrivalszkyP FrivalszkyP commented Apr 28, 2020

Closes #5960

There is an alertdialog role in ARIA which is described as this:

Dialogs are most often used to prompt the user to enter or respond to information. A dialog that is designed to interrupt workflow is usually modal.

Much like role="alert", it will be read out by screen reader software immediately when displayed.

Changelog

New

  • Added the alert prop to the react implementation of Modal.js
  • Added a unit test covering it

Testing / Reviewing

See Carbon React Storybook where I added a knob for controlling the alert prop.

@FrivalszkyP FrivalszkyP requested review from a team as code owners April 28, 2020 12:44
@ghost ghost requested review from dakahn and emyarod April 28, 2020 12:44
@FrivalszkyP
Copy link
Contributor Author

My decision behind adding a new prop to control this behavior was that, although role="alertdialog" is closely related to danger, I assume there could be cases where a dialog has the alert=true prop but not the danger=true prop. Also, I did not want to introduce a breaking change for developers who already implemented a workaround for the lack of control of the role attribute.

@netlify
Copy link

netlify bot commented Apr 28, 2020

Deploy preview for carbon-elements ready!

Built with commit 057618c

https://deploy-preview-5955--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 28, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit c43ea50

https://deploy-preview-5955--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Apr 28, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 057618c

https://deploy-preview-5955--carbon-components-react.netlify.app

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

this looks good to me, but is there a ticket associated with this PR? just trying to get some additional context for this change

@FrivalszkyP
Copy link
Contributor Author

this looks good to me, but is there a ticket associated with this PR? just trying to get some additional context for this change

Sorry, I completely glanced over the note in the guidelines that I should have created an issue. I can create one to provide some example codes.

@FrivalszkyP
Copy link
Contributor Author

@emyarod Added the issue and referenced it in my PR description! See here #5960

Hope I was able to provide enough context!

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Hi @FrivalszkyP thank you for jumping in! After comparing the purpose of alertdialog role and our modal usage guide, I'm inclined to believe that the alertdialog should be applied to all passiveModal variants. What do you think?

@dakahn
Copy link
Contributor

dakahn commented Apr 29, 2020

@asudoh the attritbute role="alertdialog" should only be applied to dialogs that require the user to respond somehow (like with a confirmation or some other interaction). Our "passive" modals should just be role="alert"

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

We may need to distinguish passive or active modals here. With passive modals not needing a users response simply being role="alert" and active modals requiring a response being role="alertdialog" since there is this distinction in our design guidance.

https://www.digitala11y.com/alertdialog-role/

@FrivalszkyP
Copy link
Contributor Author

@dakahn Good call, do you suggest changing the role to alert in case passiveModal and alert are both true?

@FrivalszkyP
Copy link
Contributor Author

Thanks! I updated the PR with the role="alert" functionality.

@tw15egan tw15egan requested a review from dakahn July 22, 2020 18:24
Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks a lot for taking this on. 🏄

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM as well, thanks for adding! 👍 ✅

@kodiakhq kodiakhq bot merged commit 825f03a into carbon-design-system:master Jul 22, 2020
@FrivalszkyP FrivalszkyP deleted the feature/alertdialog branch July 23, 2020 06:32
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.

The React implementation of the Modal component should support role="alertdialog"
5 participants