-
Notifications
You must be signed in to change notification settings - Fork 6
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
Feat/nds 253 combobox tokens #1243
Conversation
@@ -31,7 +35,8 @@ const FieldToken = ({ label, onDismiss = noop, testId }) => { | |||
role="button" | |||
aria-label={`Remove ${label}`} | |||
tabIndex={0} | |||
onClick={() => { | |||
onClick={(e) => { | |||
e.stopPropagation(); |
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.
We don't want this click to bubble up to any parent, as it may trigger another event. The dismissal click should be isolated to FieldToken
import React from "react"; | ||
import PropTypes from "prop-types"; | ||
|
||
const MultiSelectItem = ({ children }) => <>{children}</>; |
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.
This component gathers information about items so we can have an interface like this:
<MultiSelect>
<MultiSelect.Item>My item jsx</MultiSelect.Item>
</MultiSelect>
Instead of accepting a big data structure like this: <MultiSelect options={{ ...bigJsObject }} />
.
I'm exposing this Thing.Item
style of interface for two reasons:
- This would be consistent with other components, like
Select
andTabs
- This gives consumers full control over what gets rendered in each item
role="option" | ||
aria-selected={isSelected(selectedItems, item).toString()} |
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.
When using a useMultiSelect
in combination with a useSelect
, we get the incorrect aria-selected
value on items in the listbox. Here I'm just overriding what the prop getter returns, using my own helper to determine if the item is selected or not.
The role
prop is only needed here to expose the linter to it (it's otherwise added by getItemProps
)
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.
tested this locally and it works and is keyboard accessible 🥳
📘 Storybook Preview Available 👀View the Storybook build from this PR in your browser: (This action will publish a new comment and url if this PR is modified) |
🎉 This PR is included in version 3.37.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
DropdownTrigger
to accept start and end content (useful for managing tokens)MultiSelect
componentFieldToken
component, so it works within a dropdown triggeruseMultipleSelection
This hook from downshift is a state manager for more than one
selectedItem
, becauseuseSelect
only manages a singleselectedItem
.I'm using state reducers in both hooks to control the behavior we want when users interact with the options list OR the chips in the dropdown trigger.