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(multiselect): allow disabled listbox items #9859

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Oct 13, 2021

Closes #9610
Refs #6477

Modifies MultiSelect to pass through the disabled option on items to downshift. Adds new styles to properly display disabled items.

Changelog

New

  • multiselect: added listbox disabled item styles

Changed

  • multiselect: enable disabled option for listbox items

Testing / Reviewing

  • View the carbon-components-react storybook default MultiSelect story. The third option in the listbox should have the disabled attribute and appear disabled.
  • View the carbon-react-next storybook default MultiSelect story. The third option in the listbox should have the disabled attribute and appear disabled.
  • Would be awesome to get a visual review to make sure the disabled styles I added are good

@tay1orjones tay1orjones requested a review from a team as a code owner October 13, 2021 21:43
@netlify
Copy link

netlify bot commented Oct 13, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 390a87e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6170792887e7d200080752db

😎 Browse the preview: https://deploy-preview-9859--carbon-react-next.netlify.app

@tay1orjones
Copy link
Member Author

A few notes regarding the accessibility of this:

  • The disabled: true is passed through to Downshift. Downshift applies a disabled attribute to the element with role="option"
  • The option role inherits aria-disabled, which to my understanding can be used interchangeably with the disabled attribute.
  • Disabled options do not receive focus and are skipped when using keyboard navigation. From the aria-disabled spec:
    • (aria-disabled) Indicates that the element is perceivable but disabled, so it is not editable or otherwise operable ... Disabled elements might not receive focus from the tab order. For some disabled elements, applications might choose not to support navigation to descendants.

Overall I think we're within bounds accessibility-wise. @dakahn @mbgower definitely would appreciate any critique you might have of my thinking here!

@netlify
Copy link

netlify bot commented Oct 13, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 390a87e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6170792864c33100070e6059

😎 Browse the preview: https://deploy-preview-9859--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 13, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 390a87e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61707928c6b085000833afcc

😎 Browse the preview: https://deploy-preview-9859--carbon-components-react.netlify.app

@dakahn
Copy link
Contributor

dakahn commented Oct 14, 2021

Yep downshift does its job when it comes to disabled. Nice! I'd actually recommend aria-disabled over using disabled in general because the latter can cause a confusing experience potentially.

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.

Totally agree with keeping the disabled item focusable -- good call there. Looks good 👍🏾

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

The disabled text and icon should be color token $disabled-02, not -03.

@tay1orjones
Copy link
Member Author

@laurenmrice thanks for catching that! Just updated it.

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

lgtm!

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.

[Feature Request]: To disable the options in MultiSelect
3 participants