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): add role option #4670

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danilopiazza
Copy link

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md and DEVELOPER.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

This PR should close #4101.

@danilopiazza
Copy link
Author

I have a few remaining questions:

  • API documentation: which version should I document in the @since tag for the new property?
  • Demo: what should I showcase in the demo? Should I print the role attribute from the modal window, just to see if it is woking? It looks like I would be duplicating the end-to-end tests.
  • Should the new role option be updatable, like ariaDescribedBy and ariaLabelledBy?
  • At this time, the only possible values for role are alertdialog and dialog. Should the property be strictly typed, allowing these two values only, or should it be extensible, allowing any string value (like fullscreen and size)?

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.

missing configuration for attribute role at NgbModal
1 participant