-
Notifications
You must be signed in to change notification settings - Fork 97
feat: add refactored Combobox component
#1559
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
Conversation
Francois-Esquire
left a comment
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.
views often represent compositions of a user interface rather than a component (or its primitives). Views tend to be associated with application level thinking (like the MVC pattern) as well. What was the reason for src/views (instead of the conventional src/styled)?
@Francois-Esquire in light of future development plans, I want to establish a new/improved dev structure that more closely aligns with the documented API. In Garden, we historically refer to a "container-view-element" architecture. This structure reinforces that and takes weight off of the current choice of |
geotrev
left a comment
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.
No strong thoughts so far. The code looks solid. I'll take a look at the tests & storybook next.
|
Here are a couple of things I noticed:
Is there an enhancement opportunity where the keyboard can move the focus between tags with the arrow keys? In situations where you have multi-select + editable, it could be appropriate. 🤔 |
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.
Solid work @jzempel 🙌 , left some feedback.
I noticed some visual tug-o-war between the scroll bar and the "Trigger" wrapping the input and tags. When multi-select is enabled with 2 or more tags and limited space, I'm seeing some overlap which may be undesirable:
packages/dropdowns.next/demo/~patterns/stories/ListboxStory.tsx
Outdated
Show resolved
Hide resolved
| const tagProps: Record<string, IOptionProps['tagProps']> = {}; | ||
| const retVal = toOptions(children, tagProps); | ||
|
|
||
| setOptionTagProps(value => ({ ...value, ...tagProps })); |
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.
What was the reason for spreading the previous state?
A small concern I have about spreading the previous state is when it comes dynamic options, which is an edge case. It would result in persisting stale values and a possible memory leak.
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.
Without this, tagProps get lost when options are filtered out – resulting in visual changes to currently selected tags. After careful consideration, I decided a "leak" wasn't a concern as the object will remain stable at N total options.
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.
Well, what's the intended approach with virtual lists?
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.
Virtualization (which isn't a recommended a11y approach here) doesn't change the situation. The state remains consistent at N total optionTagProps, where N = as many options that have tagProps as the user has experienced.
I'm thinking about putting a condition here to only set the state if the combobox isMultiselectable. Although, it would be silly for a consumer to add tagProps to a non-multiselectable combobox.
|
@lucijanblagonic thanks for the review. Here are replies to your comments above.
I'm digging into this and will re-request your review when I have a solution committed.
I hadn't merged in the latest
Fixed in latest deploy.
I actually have this commented as a TODO in the underlying container 😉. We can explore this option as a follow-on. But, since the keyboard interaction is optional, I think the default should match what we currently have in Garden.
I don't think so. This results in an accessibility violation with the current |
I'm not super concerned about this as the native scrollbar is better left alone – and isn't present by default (without enabling in system settings). |
|
@lucijanblagonic the latest commit addresses...
PR is ready for your re-review |
lucijanblagonic
left a comment
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.
Everything looks good! 🙌






Description
Garden's current suite of combobox-like components –
Autocomplete,Combobox,MultiselectandSelect– is plagued with deep-seated accessibility issues, costly support maintenance, and APIs that challenging to navigate. This PR adds a new@zendeskgarden/dropdowns.nextpackage with a fully refactoredComboboxthat is built to replace the existing component set with 100% WCAG 2.1 and WAI-ARIA 1.2 accessibility. The non-accessible component set will be removed in a future major release.Detail
Key capabilities include:
Comboboxfunctions in both uncontrolled andcontrolled modes. Controlled mode enables aspects, such as input value, selection value(s),
listbox expansion, and current option active index, to share and adapt to the
surrounding UI.
Comboboxwith listautocomplete. Filtering implementation is left to the API consumer.
ComboboxAPI ensures the selection of one or morelistbox option values, while also supporting the W3C no autocomplete
example for use cases like search.
Comboboxto provide WAI-ARIAmulti-select listbox functionality with option-as-tag value rendering.
Comboboxsupports select-only mode, where the user cannot modify the<input>.Comboboxoffers various filtering methods for listboxoptions. Details of the filtering implementation are left to the API consumer.
Comboboxcan convert input value text to rich HTMLmarkup on blur in single-selection mode.
Comboboxallows adding start and end media (SVG icons).Certain features will replace end media with Garden's standard dropdown chevron
treatment.
ComboboxAPI utilizes fully accessible<OptGroup>components for grouping, similar to the corresponding HTML element.
Comboboxsupports compactsizing.
Comboboxbuilds on Garden’s Field API context toestablish accessible relationships with corresponding Label, Hint, and Message
components.
Comboboxprovides validation styling andaccessibility comparable to other Garden form components.
left-to-right and right-to-left layouts.
Checklist
yarn start)?bedrock)