-
Notifications
You must be signed in to change notification settings - Fork 12
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
Dropdown #204
Dropdown #204
Conversation
7793cfe
to
5fbd32c
Compare
Deploying compound-web with Cloudflare Pages
|
dada44e
to
04fcb27
Compare
04fcb27
to
d1dfe94
Compare
800a0dd
to
fc5f6e4
Compare
fc5f6e4
to
e629fc8
Compare
889a2f4
to
e94ee1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dropdown does not comply with the standard keyboard accessibility for dropdowns using arrow keys. See https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/ for an example
0841378
to
9595502
Compare
9595502
to
c4a2d33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A11y it is nearly there, selecting an option drops your focus where it should really keep your focus on the dropdown as exemplified in the example I linked you to
9b3318c
to
36d222b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good, sorry noticed one more a11y bug. When you open the dropdown via the keyboard the focus should be on the selected item, right now focus is left on the trigger, and you have to navigate manually to the selected item. The earlier WAI ARIA example page does this correctly
What do you mean by "When you open the dropdown via the keyboard the focus should be on the selected item"? Which selected item? When the user has already selected an item in the dropdown previously and opens it a second time? |
Yes exactly. See recording for unexpected & expected behaviour Screen.Recording.2024-08-06.at.17.19.42.movScreen.Recording.2024-08-06.at.17.20.08.mov |
b3cd794
to
6782688
Compare
src/components/Dropdown/Dropdown.tsx
Outdated
// Focus the item if the dropdown is open and the item is already selected | ||
useEffect(() => { | ||
if (isSelected && isDisplayed) { | ||
ref?.current?.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref?.current?.focus(); | |
ref.current?.focus(); |
Isn't ref never falsy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep you're right. It's a remnant of the previous forward ref hoc :)
Add a dropdown component.
https://www.figma.com/design/rTaQE2nIUSLav4Tg3nozq7/Compound-Web-Components?node-id=799-5732&t=pBlESNonZu5jMpV0-0
I didn't use select/option to be able to design it to match the figma. Also I don't put it to the form because we are not using input html element.
I tried to use an hidden select/input and set the value manually to make the component work as other form component however if we change manually the value of an input, the change synthetic event is not fired. If we fire manually the change event, the onChange listener will not listen it because it's not a synthetic event.