Skip to content

Conversation

halocline
Copy link
Collaborator

@halocline halocline commented May 4, 2022

What does this PR do?

Closes #6088

Add disabled prop to List.

  • Add prop allowing array of indexes to be supplied by caller.
  • Disabled items should be included in the list length and are visually styled as disabled at the specified indices.
  • Because disabled items are in the list length and a visually perceivable, we need to replicate this behavior for screen reader users.
    • As example, consider presenting a product on a product detail page. That product may have attributes such as color and size. The color and sizes options are presented in a list so that the user knows what options are available. However, at certain points in time, a particular color or size may be out of stock and the interface will want to display that item as disabled, indicating that that item is normally carried, but currently out of stock.
    • Ensure that disabled items are included in the "item X of Y" count. Identify list items as disabled to screen readers.aria-disabled="true is preferred over the disabled attribute as it includes the item in the count, allows for focus, and announces "disabled" or "dimmed" depending on the browser and screen reader.
  • onClick and onEnter events should be prevented for disabled items.
  • Treatment of disabled items when List is in onOrder state:
    • Disabled items should be included in the onOrder list.
    • Disabled items should retain disabled styling and aria-disabled, however, the items may be re-ordered.
    • Disabled items should be able to be re-ordered. Disabled indices should be updated to reflect the new order.
      • As example, consider a list of rules which are to be applied in a certain order, such as a lead quality filter regulating the amount of leads being distributed to a sales team. As circumstances change, the order of the rules may want to be modified and/or certain rules may want to be temporarily disabled.

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

  • basic disabled
  • basic disabled with screen reader
  • disabled + onClick
  • disabled + onClick + keyboard
  • disabled + onClick + screen reader
  • disabled + onOrder
  • disabled + onOrder + keyboard
  • disabled + onOrder + screen reader

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent. 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.

@halocline halocline requested review from britt6612 and jcfilben May 4, 2022 21:34
Copy link
Collaborator

@jcfilben jcfilben left a 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

@halocline halocline changed the title Feat 6088 list disabled prop List - add disabled prop May 4, 2022
@halocline
Copy link
Collaborator Author

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.

Copy link
Collaborator

@jcfilben jcfilben 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, 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.

@britt6612 britt6612 self-requested a review May 7, 2022 01:05
@britt6612 britt6612 requested a review from ericsoderberghp May 9, 2022 14:12
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.

List — Add disabled prop that takes array of disabled items
4 participants