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

Dropdown #204

Merged
merged 26 commits into from
Aug 7, 2024
Merged

Dropdown #204

merged 26 commits into from
Aug 7, 2024

Conversation

florianduros
Copy link
Member

@florianduros florianduros commented Jul 5, 2024

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.

@florianduros florianduros force-pushed the florianduros/dropdown branch from 7793cfe to 5fbd32c Compare July 5, 2024 13:55
Copy link

cloudflare-workers-and-pages bot commented Jul 5, 2024

Deploying compound-web with  Cloudflare Pages  Cloudflare Pages

Latest commit: c9ada96
Status: ✅  Deploy successful!
Preview URL: https://2db48043.compound-web.pages.dev
Branch Preview URL: https://florianduros-dropdown.compound-web.pages.dev

View logs

@florianduros florianduros force-pushed the florianduros/dropdown branch 8 times, most recently from dada44e to 04fcb27 Compare July 5, 2024 14:58
@florianduros florianduros force-pushed the florianduros/dropdown branch from 04fcb27 to d1dfe94 Compare July 5, 2024 15:37
@florianduros florianduros force-pushed the florianduros/dropdown branch from 800a0dd to fc5f6e4 Compare July 8, 2024 14:38
@florianduros florianduros force-pushed the florianduros/dropdown branch from fc5f6e4 to e629fc8 Compare July 8, 2024 14:57
@florianduros florianduros force-pushed the florianduros/dropdown branch from 889a2f4 to e94ee1f Compare July 10, 2024 15:47
@florianduros florianduros marked this pull request as ready for review July 10, 2024 16:04
@florianduros florianduros requested a review from a team as a code owner July 10, 2024 16:04
@florianduros florianduros requested review from t3chguy and robintown and removed request for a team July 10, 2024 16:04
Copy link
Member

@t3chguy t3chguy left a 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

@florianduros florianduros force-pushed the florianduros/dropdown branch from 0841378 to 9595502 Compare July 22, 2024 16:58
@florianduros florianduros force-pushed the florianduros/dropdown branch from 9595502 to c4a2d33 Compare July 22, 2024 16:59
@florianduros florianduros requested a review from t3chguy July 23, 2024 07:11
Copy link
Member

@t3chguy t3chguy left a 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

src/components/Dropdown/Dropdown.module.css Show resolved Hide resolved
src/components/Dropdown/Dropdown.test.tsx Outdated Show resolved Hide resolved
src/components/Dropdown/index.ts Outdated Show resolved Hide resolved
@florianduros florianduros force-pushed the florianduros/dropdown branch from 9b3318c to 36d222b Compare August 6, 2024 15:00
@florianduros florianduros requested a review from t3chguy August 6, 2024 15:10
Copy link
Member

@t3chguy t3chguy left a 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

@florianduros
Copy link
Member Author

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?

@t3chguy
Copy link
Member

t3chguy commented Aug 6, 2024

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.mov
Screen.Recording.2024-08-06.at.17.20.08.mov

@florianduros florianduros force-pushed the florianduros/dropdown branch from b3cd794 to 6782688 Compare August 7, 2024 08:40
@florianduros florianduros requested a review from t3chguy August 7, 2024 08:44
// Focus the item if the dropdown is open and the item is already selected
useEffect(() => {
if (isSelected && isDisplayed) {
ref?.current?.focus();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ref?.current?.focus();
ref.current?.focus();

Isn't ref never falsy?

Copy link
Member Author

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 :)

@florianduros florianduros merged commit ab7ecc5 into main Aug 7, 2024
6 checks passed
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