-
Notifications
You must be signed in to change notification settings - Fork 12
feat(chip): add selection mode and toggle behavior #2764
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
base: main
Are you sure you want to change the base?
Changes from all commits
33e0dc4
03d35cf
951a788
a6f3302
22e61d4
104879e
22dcf74
726d218
ce91f32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "minor", | ||
| "comment": "Adds support for selection-mode on the nimble-chip component to allow for toggleable chips.", | ||
| "packageName": "@ni/nimble-components", | ||
| "email": "1458528+fredvisser@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,10 +64,13 @@ _The key elements of the component's public API surface:_ | |
| - `removable` - set to show the remove button | ||
| - `appearance` - supports `outline` and `block` appearances | ||
| - `disabled` - styles the chip in the typical Nimble manner, including any user-slotted content (text and icon), and additionally hides the remove button. | ||
| - `selection-mode` - controls whether the chip can be selected. Values: `'none'` (default) or `'single'`. When `'single'`, the chip acts as a toggle button with `role="button"` and `aria-pressed`. | ||
| - `selected` - indicates the current selection state when `selection-mode="single"`. When `true`, the chip displays with selected background styling. | ||
| - _Methods_ | ||
| - _None_ | ||
| - _Events_ | ||
| - `remove` - fired when the chip remove button is pressed. | ||
| - `remove` - fired when the chip remove button is pressed, or when Escape key is pressed on a removable chip. | ||
| - `selected-change` - fired when the user toggles the chip's selected state via click or keyboard (Space/Enter). Only emitted when `selection-mode="single"`. | ||
| - _Slots_ | ||
| - `start` - icon placed to the left of the chip text | ||
| - (default) - for the primary label text | ||
|
|
@@ -145,12 +148,18 @@ We will provide styling for the `disabled` attribute state. | |
| _Consider the accessibility of the component, including:_ | ||
|
|
||
| - _Keyboard Navigation and Focus_ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you update the API section above to include the new APIs too? The other md file seems more like copilot instructions for this specific feature but it's still helpful to keep the entire API summarized in this higher level spec. |
||
| - when the chip component is removable, the remove button will be focusable, otherwise it will not receive focus (following the `nimble-banner` pattern). | ||
| - When the chip is selectable (`selection-mode="single"`) and removable: | ||
| - The chip itself is focusable and receives keyboard events | ||
| - Space/Enter toggles the selected state | ||
| - Escape removes the chip (emits `remove` event) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you consider whether Escape should also emit the event when the chip is not selectable? I'd advocate for it to work in both cases unless there's a reason not to. |
||
| - The remove button is **not** focusable (`tabindex="-1"`) to avoid nested interactive controls (violates [WCAG 4.1.2](https://dequeuniversity.com/rules/axe/4.11/nested-interactive)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is unclear to me. Is it saying that this proposal violates the rule and we're ok with it or that this choice is made to avoid violating that rule? If it's the former, what's the justification for violating the rule? |
||
| - When the chip is removable but not selectable (`selection-mode="none"`): | ||
| - The remove button is focusable and can be activated with Space or Enter | ||
| - _Form Input_ | ||
| - N/A | ||
| - _Use with Assistive Technology_ | ||
| - When selectable, the chip has `role="button"` and `aria-pressed` to indicate its toggle state | ||
| - a `chip`'s accessible name comes from the element's contents by default | ||
| - no ARIA `role` seems necessary to define for the chip, as it isn't interactive itself (only the remove button is which has a `role`). The only valid role seemed to be [`status`](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/status_role), but that also didn't seem helpful from an accessibility perspective, particularly since it mainly relates to providing helpful information when the content changes (which we don't expect). | ||
| - the remove button will have its content set to provide a label provider token for "Remove". | ||
| - title will not be set, which aligns with decisions for the filterable select clear button and the banner | ||
| - ideally this would include the contents of the chip itself (so a screen reader would announce "Remove <Chip>") but differing word order between | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a Future Work section below that mentions making the chip selectable which could be removed now. |
||
|
|
@@ -160,8 +169,6 @@ _Consider the accessibility of the component, including:_ | |
|
|
||
| ### Future work | ||
|
|
||
| - Make chip selectable (there are already UX designs) | ||
| - Currently there are no use-cases for chips requiring them to be selectable, but there are many use-cases in the wild where this is needed. | ||
| - Provide error state for the chip (there are already UX designs) | ||
| - Again, there are no current use-cases requiring a chip to present with error information, but it is not unreasonable to expect we may have such a use-case in the future. | ||
| - Create a chip container component that manages chip layout, and removal | ||
|
|
||
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.
'single'implies there might someday be a'multiple'. I'm not sure about how that would work though. Right now we only have individual chips that are not aware of other chips. We don't have a container component like the table is for rows or the radio group is for radio buttons. Without that, I'm not sure what would enforce how many chips can be selected and thus I'm not sure it makes sense for clients to specify it via the API on the chip. I don't want to imply that Nimble will coordinate selection state across chips somehow.Some options:
selection-modeAPI to the parent component. But then we'd need to figure out other aspects of its behavior like sizing / layout / spacing.selectablefor now. If it later becomes important for apps to enforce single vs multiple selection mode, we'd have some options: