-
Notifications
You must be signed in to change notification settings - Fork 1k
List - add disabled prop #6111
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
List - add disabled prop #6111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While using a screenreader I am able to select disabled options and the screen reader will read "list item dimmed, selected (2 of 5)". My expectation would be that I shouldn't be able to select disabled items
Addressed. |
There was a problem hiding this 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, thanks for adding jest tests!
One thing to note is on Chrome and Firefox while using voice over, options that are not disabled are read as selected even if we haven't selected them. This is the current behavior on the master branch and I think fixing this falls outside the scope of this PR, so I am fine with leaving this as is for now. Just something to note that we should fix later.
What does this PR do?
Closes #6088
Add
disabled
prop to List.aria-disabled="true
is preferred over thedisabled
attribute as it includes the item in the count, allows for focus, and announces "disabled" or "dimmed" depending on the browser and screen reader.aria-disabled
, however, the items may be re-ordered.Where should the reviewer start?
stories/List/Disabled
What testing has been done on this PR?
Jest tests added.
How should this be manually tested?
stories/List/Disabled
Do Jest tests follow these best practices?
screen
is used for querying.userEvent
is used in place offireEvent
. userEvent.keyboard not behaving as expected.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
#6088
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Yes.
Should this PR be mentioned in the release notes?
Yes.
Is this change backwards compatible or is it a breaking change?
Backwards compatible.