feat(chip): add selection mode and toggle behavior #2764
Conversation
fredvisser
force-pushed
the
users/fvisser/chip-toggle
branch
from
fc883c7 to
33e0dc4
Compare
jattasNI
left a comment
jattasNI
left a comment
There was a problem hiding this comment.
Just reviewed API design, proposed behavior, and document structure so far, not actual implementation.
|
|
||
| _Consider the accessibility of the component, including:_ | ||
|
|
||
| - _Keyboard Navigation and Focus_ |
There was a problem hiding this comment.
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.
| - The chip itself is focusable and receives keyboard events | ||
| - Space/Enter toggles the selected state | ||
| - Escape removes the chip (emits `remove` event) | ||
| - 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)) |
There was a problem hiding this comment.
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 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) |
There was a problem hiding this comment.
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.
| - 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 |
There was a problem hiding this comment.
There's a Future Work section below that mentions making the chip selectable which could be removed now.
| import { withActions } from 'storybook/internal/actions/decorator'; | ||
| import { chipTag } from '@ni/nimble-components/dist/esm/chip'; | ||
| import { ChipAppearance } from '@ni/nimble-components/dist/esm/chip/types'; | ||
| import { |
There was a problem hiding this comment.
This PR should include updates to the matrix story.
| - `none`: The chip is not selectable. Does not have `role="button"` or `aria-pressed`. User-supplied `tabindex` is forwarded to the remove button if removable. | ||
| - `single`: The chip can be toggled on/off via click or Space/Enter keys. Has `role="button"` and `aria-pressed`. Automatically receives `tabindex="0"` unless user provides a different value. | ||
| - **Attribute:** `selected` (boolean, default: `false`) | ||
| - Indicates the current selection state when `selection-mode="single"`. |
There was a problem hiding this comment.
Is the intent that the attribute should reflect the interactive state? That doesn't seem to be working in the current implementation. If that's not the intent, it'd be good to document and justify.
| - `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`. |
There was a problem hiding this comment.
'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:
- take the time to design that parent component and move the
selection-modeAPI to the parent component. But then we'd need to figure out other aspects of its behavior like sizing / layout / spacing. - change this to a boolean attribute like
selectablefor now. If it later becomes important for apps to enforce single vs multiple selection mode, we'd have some options:- provide instructions for apps to implement single vs multi select patterns themselves. (It might require adding some new cancelable "beforeselection" event, I haven't thought it through)
- build a Nimble container component with an API to specify selection behavior and make a breaking change to remove the attribute from the chip if it's now redundant.
| - use the `medium-padding` token to create horizontal and vertical space between | ||
| chips | ||
| - all chips have the same appearance | ||
|
|
There was a problem hiding this comment.
It'd be good to think through the feedback from the demo meeting about the difference between a selectable (non-removable) chip and a toggle button. What use cases is each one best suited for? Then we could add those docs to each component and consider cross linking them.
Pull Request
🤨 Rationale
Adds support for
selection-modeon thenimble-chipcomponent to allow for toggleable chips. This enables use cases where chips act as filter toggles or selectable items.👩💻 Implementation
selectionModeattribute toChipcomponent.selectedstate and styling.tabindexto ensure proper accessibility for selectable chips.🧪 Testing
✅ Checklist