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

[select] fix: ARIA listbox popup type and combobox list role #5348

Merged
merged 34 commits into from
Jun 6, 2022

Conversation

bvandercar-vt
Copy link
Contributor

@bvandercar-vt bvandercar-vt commented Jun 2, 2022

Fixes #0000

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Apply role="listbox" for the Menu created by QueryList-- Menu has role="menu" by default, and listbox is more applicable here. Also apply role="combobox" to the item that creates the listbox popover

Trying to model after https://www.digitala11y.com/combobox-role/ for Select2 and MultiSelect2, https://www.w3.org/TR/2021/NOTE-wai-aria-practices-1.2-20211129/examples/combobox/combobox-autocomplete-list.html for Suggest2.

with menuProps, user can pass a custom id (or other props) to the Menu created within the Select/Multiselect popover.

@bvandercar-vt bvandercar-vt changed the title fix list role fix list roles Jun 2, 2022
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

can you please add these attributes for Suggest2 as well? thanks

@adidahiya adidahiya changed the title fix list roles [select] fix: ARIA listbox popup type and combobox list role Jun 3, 2022
@bvandercar-vt
Copy link
Contributor Author

can you please add these attributes for Suggest2 as well? thanks

Updated Suggest2 to match the suggest combobox example here https://www.w3.org/TR/2021/NOTE-wai-aria-practices-1.2-20211129/examples/combobox/combobox-autocomplete-list.html

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Almost there... thanks for iterating with me on this.

I don't like to use the term "wrapper" in the API; it's a little too ambiguous. And besides we had a "wrapper" element in Popover v1 which we've gotten rid of in Popover2, so I don't want to re-introduce that term.

packages/select/src/components/select/select2.tsx Outdated Show resolved Hide resolved
packages/select/src/components/select/select2.tsx Outdated Show resolved Hide resolved
packages/select/src/components/select/select2.tsx Outdated Show resolved Hide resolved
packages/select/src/components/select/select2.tsx Outdated Show resolved Hide resolved
bvandercar-vt and others added 7 commits June 3, 2022 12:26
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
@adidahiya
Copy link
Contributor

Quick note for using Github's suggestions feature - it's possible to batch suggestions into a single commit, see the docs here. Not a big deal or important for my review, just something for your convenience.

@bvandercar-vt
Copy link
Contributor Author

Quick note for using Github's suggestions feature - it's possible to batch suggestions into a single commit, see the docs here. Not a big deal or important for my review, just something for your convenience.

Thanks, great tip!! I also re-ordered the props in alphabetical order given the change

@adidahiya
Copy link
Contributor

Latest code looks great.

with menuProps, user can pass a custom id (or other props) to the Menu created within the Select/Multiselect popover.

do you still need this feature now that we auto-generate the ID?

@bvandercar-vt
Copy link
Contributor Author

Latest code looks great.

with menuProps, user can pass a custom id (or other props) to the Menu created within the Select/Multiselect popover.

do you still need this feature now that we auto-generate the ID?

yes we need it so that we can pass the id prop to the Menu created in the queryList

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

cool, thanks for removing that extra addition to the public API. I am open to adding it in the future if necessary, but I prefer to be conservative about adding to the API for now.

@adidahiya adidahiya merged commit 6d259b4 into palantir:develop Jun 6, 2022
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.

2 participants