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

fix(ListBoxTrigger): support title on ListBoxTrigger #8094

Merged

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Mar 15, 2021

related #8091

This PR adds title support to the ComboBox ListBoxTrigger and adds Storybook knobs to Dropdown and ComboBox for using translateWithId (matching the multiselect story)

Changelog

New

  • title on ListBoxTrigger
  • storybook knobs for dropdown and combobox

Testing / Reviewing

Ensure the translateWithId storybook knobs work correctly for the listbox components

@netlify
Copy link

netlify bot commented Mar 15, 2021

Deploy preview for carbon-elements ready!

Built with commit ea91eda

https://deploy-preview-8094--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Mar 15, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit ea91eda

https://deploy-preview-8094--carbon-components-react.netlify.app

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Thanks for adding that in so quick 🚀 👍🏻

@@ -36,6 +36,7 @@ const ListBoxTrigger = ({ isOpen, translateWithId: t, ...rest }) => {
<button
{...rest}
aria-label={description}
title={description}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for the trigger we removed title based on one of Carolyn's suggestions. It seems like having both would basically repeat the same information twice for a screen reader user. I think an alternative she suggested was to use only title if we need to offer it (remove the `aria-label in this case). This was the comment IIRC: #6938 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

it is ok to keep the <title>Close menu</title> for sighted mouse users, but please change it to <title>Close</title> (which is less verbose, and removes the confusing word "menu")

are you referring to this point?

on a side note, she and I briefly discussed this for #4185 / #8090 and from testing the combobox, multiselect, number input, etc it doesn't seem to be double speaking (in VO at least)

image

the current multiselect places the title under the SVG rather than the button itself, so that may be an option as well but right now it doesn't seem like either approach is causing VO to read twice

Copy link
Contributor

Choose a reason for hiding this comment

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

@emyarod I'm not sure which screen reader it shows up in, just was going off of what Carolyn shared. Just was sharing that if we needed the title then we could just drop the aria-label altogether based on what she shared.

@emyarod emyarod requested a review from joshblack March 16, 2021 15:02
@kodiakhq kodiakhq bot merged commit 8d39b78 into carbon-design-system:main Mar 17, 2021
@emyarod emyarod deleted the 8091-listbox-translatewithid branch March 17, 2021 18:07
@emyarod emyarod mentioned this pull request Mar 22, 2021
87 tasks
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.

5 participants