-
Notifications
You must be signed in to change notification settings - Fork 11
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 5 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": "[email protected]", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,12 +145,18 @@ We will provide styling for the `disabled` attribute state. | |
| _Consider the accessibility of the component, including:_ | ||
|
|
||
| - _Keyboard Navigation and Focus_ | ||
| - 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. |
||
|
|
||
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.
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.