Skip to content

feat(multiselect): allow disabled listbox items#9859

Merged
kodiakhq[bot] merged 4 commits into
carbon-design-system:mainfrom
tay1orjones:9610-disabled-multiselect-options
Oct 20, 2021
Merged

feat(multiselect): allow disabled listbox items#9859
kodiakhq[bot] merged 4 commits into
carbon-design-system:mainfrom
tay1orjones:9610-disabled-multiselect-options

Conversation

@tay1orjones

@tay1orjones tay1orjones commented Oct 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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 dakahn October 13, 2021 21:43
@tay1orjones tay1orjones requested a review from a team as a code owner October 13, 2021 21:43
@tay1orjones tay1orjones requested a review from aledavila October 13, 2021 21:43
@netlify

netlify Bot commented Oct 13, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ 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

tay1orjones commented Oct 13, 2021

Copy link
Copy Markdown
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!

@tay1orjones tay1orjones requested review from a team and laurenmrice and removed request for a team October 13, 2021 22:04
@netlify

netlify Bot commented Oct 13, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ 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

netlify Bot commented Oct 13, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ 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

dakahn commented Oct 14, 2021

Copy link
Copy Markdown
Contributor

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.

dakahn
dakahn approved these changes Oct 14, 2021
View reviewed changes

@dakahn dakahn left a comment

Copy link
Copy Markdown
Contributor

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 👍🏾

laurenmrice
laurenmrice requested changes Oct 19, 2021
View reviewed changes

@laurenmrice laurenmrice left a comment

Copy link
Copy Markdown
Member

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

tay1orjones commented Oct 20, 2021

Copy link
Copy Markdown
Member Author

@laurenmrice thanks for catching that! Just updated it.

laurenmrice
laurenmrice approved these changes Oct 20, 2021
View reviewed changes

@laurenmrice laurenmrice left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm!

@kodiakhq kodiakhq Bot merged commit 84de2dc into carbon-design-system:main Oct 20, 2021
@tay1orjones tay1orjones mentioned this pull request Oct 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurenmrice laurenmrice laurenmrice approved these changes

@aledavila aledavila Awaiting requested review from aledavila aledavila was automatically assigned from carbon-design-system/developers-system

+1 more reviewer

@dakahn dakahn dakahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status: one more review 👀 status: ready to merge 🎉

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature Request]: To disable the options in MultiSelect

3 participants

@tay1orjones @dakahn @laurenmrice