Checkbox group component#1243
Conversation
|
Hi there @bjarnef, thank you for this contribution! 👍 While we wait for the team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new uui-checkbox-group component to provide consistency with the existing uui-radio-group component and enable form validation for groups of checkboxes.
Key Changes:
- New
uui-checkbox-groupcomponent with supporting event classes - Additional methods added to
uui-checkboxelement (check(),uncheck(),makeFocusable(),makeUnfocusable()) - Package configuration, tests, stories, and documentation for the new component
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/uui/lib/index.ts | Exports the new checkbox-group component |
| packages/uui-radio/lib/uui-radio.element.ts | Minor formatting changes (added blank lines) |
| packages/uui-checkbox/lib/uui-checkbox.element.ts | Adds group-related methods (check, uncheck, focus management) |
| packages/uui-checkbox/lib/index.ts | Exports UUICheckboxEvent class |
| packages/uui-checkbox/lib/UUICheckboxEvent.ts | New event class for checkbox change events |
| packages/uui-checkbox-group/tsconfig.json | TypeScript configuration for the new package |
| packages/uui-checkbox-group/rollup.config.js | Build configuration for the new package |
| packages/uui-checkbox-group/package.json | Package metadata and dependencies |
| packages/uui-checkbox-group/lib/uui-checkbox-group.test.ts | Basic tests for the component (minimal coverage) |
| packages/uui-checkbox-group/lib/uui-checkbox-group.story.ts | Storybook examples demonstrating usage |
| packages/uui-checkbox-group/lib/uui-checkbox-group.element.ts | Main component implementation |
| packages/uui-checkbox-group/lib/index.ts | Package exports |
| packages/uui-checkbox-group/lib/UUICheckboxGroupEvent.ts | Event class for checkbox group changes |
| packages/uui-checkbox-group/README.md | Component documentation |
| package-lock.json | Dependency lock file updates |
Critical Issues Found:
The implementation has a fundamental flaw: it implements single-selection behavior (like radio buttons) when checkbox groups should support multiple simultaneous selections. The component needs significant rework to properly handle multiple checked values, correct keyboard navigation, and appropriate focus management for checkbox semantics rather than radio button semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #onSelectClick = (e: UUICheckboxEvent) => { | ||
| if (e.target.checked === true) { | ||
| this.value = e.target.value; | ||
| this.#fireChangeEvent(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The checkbox group currently implements single-selection behavior (similar to radio buttons), but checkbox groups should support multiple selections. The #onSelectClick method only updates the value when a checkbox is checked to true, and the #updateCheckboxElementsCheckedState method unchecks all other checkboxes when one is selected. This contradicts the fundamental purpose of checkboxes, which is to allow multiple items to be selected simultaneously. The value property should be an array of selected values, not a single string.
| #updateCheckboxElementsCheckedState(newValue: FormData | FormDataEntryValue) { | ||
| const notChecked: Array<UUICheckboxElement> = []; | ||
|
|
||
| this.#checkboxElements.forEach((el, index) => { | ||
| if (el.value === newValue) { | ||
| el.checked = true; | ||
| el.makeFocusable(); | ||
| this.#selected = index; | ||
| } else { | ||
| el.checked = false; | ||
| notChecked.push(el); | ||
| } | ||
| }); | ||
|
|
||
| // If there is a selected checkbox, make all other checkboxes unfocusable. | ||
| if (this.#selected !== null) { | ||
| notChecked.forEach(el => el.makeUnfocusable()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method treats checkboxes as mutually exclusive (only one can be checked at a time), which is incorrect for checkbox groups. When a checkbox value matches newValue, all other checkboxes are unchecked. This should instead support multiple checked values by accepting an array or delimited string of values and checking all matching checkboxes.
| import { html, fixture, expect } from '@open-wc/testing'; | ||
| import { UUICheckboxGroupElement } from './uui-checkbox-group.element'; | ||
|
|
||
| describe('UUICheckboxGroupElement', () => { | ||
| let element: UUICheckboxGroupElement; | ||
|
|
||
| beforeEach(async () => { | ||
| element = await fixture(html` <uui-checkbox-group></uui-checkbox-group> `); | ||
| }); | ||
|
|
||
| it('is defined with its own instance', () => { | ||
| expect(element).to.be.instanceOf(UUICheckboxGroupElement); | ||
| }); | ||
|
|
||
| it('passes the a11y audit', async () => { | ||
| await expect(element).shadowDom.to.be.accessible(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test coverage for the checkbox group is minimal. There are no tests for multiple selection behavior, value handling, keyboard navigation, or interaction between multiple checkboxes. The radio group component has comprehensive tests that should be adapted for checkbox group behavior.
| export const DefaultValue: Story = { | ||
| render: () => html` | ||
| <uui-checkbox-group value="3"> | ||
| <uui-checkbox value="1">one</uui-checkbox> | ||
| <uui-checkbox value="2">two</uui-checkbox> | ||
| <uui-checkbox value="3">three</uui-checkbox> | ||
| <uui-checkbox value="4">four</uui-checkbox> | ||
| </uui-checkbox-group> | ||
| `, | ||
| }; |
There was a problem hiding this comment.
The DefaultValue story sets value="3" on the checkbox group, which with the current single-selection implementation would only check one checkbox. For a proper checkbox group implementation that supports multiple selections, this should demonstrate setting multiple default values, for example value="1,3" or using an array format.
| } | ||
|
|
||
| /** | ||
| * Call to make the element focusable, this sets tabindex to -1. |
There was a problem hiding this comment.
The documentation comment incorrectly states that makeUnfocusable sets tabindex to -1 when it should say "make the element unfocusable". The method name already indicates this action.
| * Call to make the element focusable, this sets tabindex to -1. | |
| * Call to make the element unfocusable. |
| #selectPreviousCheckbox() { | ||
| this.value = this.#findAdjacentCheckboxElement(-1)?.value ?? ''; | ||
| this.#checkboxElements[this.#selected ?? 0]?.focus(); | ||
| this.#fireChangeEvent(); | ||
| } | ||
|
|
||
| #selectNextCheckbox() { | ||
| this.value = this.#findAdjacentCheckboxElement()?.value ?? ''; | ||
| this.#checkboxElements[this.#selected ?? 0]?.focus(); | ||
| this.#fireChangeEvent(); | ||
| } |
There was a problem hiding this comment.
The keyboard navigation with arrow keys implements single-selection behavior by selecting the previous checkbox and deselecting others. For checkbox groups, arrow keys should typically move focus without changing selection state. Space or Enter should toggle the focused checkbox's checked state while preserving other selections.
| this.#updateCheckboxElementsCheckedState(newValue as string); | ||
| } | ||
|
|
||
| #selected: number | null = null; |
There was a problem hiding this comment.
The #selected field tracks a single selected index, which is appropriate for radio groups but not for checkbox groups where multiple checkboxes can be selected simultaneously. This should either be removed or changed to track an array of selected indices if needed for focus management.
|
@iOvergaard @nielslyngsoe it seems there is a small inconsistency between For For but this does:
fe528b0 accounts for only whitespace in slotted content as
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
| import { css, html } from 'lit'; | ||
|
|
||
| //import { UUICheckboxEvent } from './UUICheckboxEvent'; | ||
|
|
There was a problem hiding this comment.
Not sure about this. Ideally checkbox emit UUICheckboxEvent as radio emit UUIRadioEvent, but checkbox inherits boolean input. Can we override change event inherited, but I guess it may possible be a breaking change - or perhaps trigger both events?
| //this.#updateCheckboxElementsCheckedState(newValue as string); | ||
| } | ||
|
|
||
| #selected: number | null = null; |
There was a problem hiding this comment.
I think this could be a problem. One thing is what Copilot mentions here, but my concern would be if the choices are updated/appended/reordered/removed while this component is running. So I would suggest finding another way to identify this need.
Also note the value would reflect what is selected?, so could that be used?
There was a problem hiding this comment.
I guess, but wouldn't it be a problem for uui-radio-group as well, as it initially used inspiration from this and if we can streamline these:





Description
I noticed we have a
uui-radio-groupcomponent, but nouui-checkbox-groupcomponent as most other webcomponent libraries I have came across:https://react-spectrum.adobe.com/v3/CheckboxGroup.html
https://storybook.heroui.com/?path=/story/components-checkboxgroup--default
https://www.radix-ui.com/themes/docs/components/checkbox-group
https://base-ui.com/react/components/checkbox-group
This can be useful for various thing, including form validation of checkbox group, but also make it a bit more consistent with radios.
The checkbox inherits from the boolean input (which toggle also inherits from), but not sure if it would be better checkbox and radio component inherits from a base component instead as these share more similaries.
Default value prorperty need to be an array instead of string as this example:
https://www.radix-ui.com/themes/docs/components/checkbox-group
I think it may be better to move
uui-radio-groupto it's own folder as this is how much other components are structured and it allows to have separated docs & stories.The keyboard constants in radio group place should probably be moved to a central place, so other components can use these as well.
Types of changes
Motivation and context
More consistent with radios and allow us to handle configuration on group itself, e.g. an
orientationproperty withvertical | horizontaloptions - defaultvertical.Easier to control gap/spacing between checkbox options (and not just personal preferences when listing checkboxes).
I have started using most similar logic as in radio group component, but some of it can be removed as it doesn't need to consider single choice as checkbox group will always be a multiple selection.
Other things could be relevant like mark as required, validation state, error message etc.
A label/description per checkbox/radio group may also be useful and from accessibility point of view.
How to test?
Screenshots (if appropriate)
Checklist