feat(checkbox)!: align onCheckedChange with Base UI, remove deprecated props#416
feat(checkbox)!: align onCheckedChange with Base UI, remove deprecated props#416mattrothenberg wants to merge 5 commits intomainfrom
Conversation
…cated props BREAKING CHANGE: onCheckedChange signature now includes eventDetails as second argument. Removed deprecated props: onChange, onValueChange, onClick
commit: |
Docs PreviewCommit: |
|
/bigbonk |
There was a problem hiding this comment.
Review
Clean PR — the intent is sound and the implementation is minimal. Removing the deprecated wrapper and passing onCheckedChange straight through to Base UI is the right call. Two issues to flag:
1. CheckboxChangeEventDetails is not reachable from the public API
The PR description advertises this import:
import { type CheckboxChangeEventDetails } from '@cloudflare/kumo';But the type is only exported from checkbox.tsx. It's missing from both barrel files:
packages/kumo/src/components/checkbox/index.ts— needstype CheckboxChangeEventDetailsadded to the export listpackages/kumo/src/index.ts(line 46) — needs it added to the re-export
Without this, consumers can't actually import the type.
2. Stale registry metadata injects a ghost onValueChange prop
packages/kumo/scripts/component-registry/metadata.ts lines 257-262 still defines:
Checkbox: {
onValueChange: {
type: "(checked: boolean) => void",
description: "Callback when checkbox value changes",
},
},This is in ADDITIONAL_COMPONENT_PROPS, which means the generated component-registry.json will advertise onValueChange as a valid prop on Checkbox even though the type no longer accepts it. This will confuse both AI consumers of the registry and the no-deprecated-props lint rule (which reads from the registry). This entry should be removed.
Minor notes
- The visual regression diffs (0.17%–1% pixel changes) look like sub-pixel rendering noise, not functional changes — expected given this is a props-only change with no styling modifications.
- The
Checkbox.Group.onValueChangeprop is correctly left alone. - The
HomeGrid.tsxupdate fromonValueChangetoonCheckedChangeis correct. - The whitespace-only reformatting of the
hover:ring-kumo-hairlineline is harmless but worth noting — it's unrelated to the feature change.
|
Review posted on PR #416. Here's a summary of the findings: Two issues flagged:
Everything else looks good — the core change (removing the wrapper and passing |
|
/bigbonk updated. can you re-review and approve now? |
There was a problem hiding this comment.
Review
Clean change overall. The core simplification (dropping the wrapper, passing onCheckedChange straight through to Base UI) is correct and the changeset is well-written.
Bonk findings triage
Finding #1 (partially valid): CheckboxChangeEventDetails is exported from checkbox/index.ts (line 5), but it's missing from the main barrel at src/index.ts:46:
export { Checkbox, type CheckboxProps } from "./components/checkbox";
// ^ needs type CheckboxChangeEventDetails hereThe PR description and changeset both advertise import { type CheckboxChangeEventDetails } from '@cloudflare/kumo' — that import will fail at the package level. Needs a one-line fix.
Finding #2 (already addressed): The stale Checkbox.onValueChange entry in ADDITIONAL_COMPONENT_PROPS was already removed in this PR (visible in the diff). Bonk may have been looking at a stale version. No action needed.
Remaining issue
Add CheckboxChangeEventDetails to the main barrel export:
- export { Checkbox, type CheckboxProps } from "./components/checkbox";
+ export { Checkbox, type CheckboxProps, type CheckboxChangeEventDetails } from "./components/checkbox";Co-authored-by: mattrothenberg <mattrothenberg@users.noreply.github.com>
|
That's a pre-existing error (the The fix is straightforward. Here's a summary: What I did:
|





































































Summary
This PR aligns Checkbox's
onCheckedChangecallback with Base UI's native signature and removes deprecated props that have been superseded.Background
Base UI's Checkbox provides
onCheckedChangewith two arguments:Kumo was wrapping this and only exposing the first argument (
checked: boolean), while maintaining three deprecated alternative props (onChange,onValueChange,onClick) for backwards compatibility. This added complexity and prevented consumers from accessing the underlying event when needed.Changes
onCheckedChangenow exposes event detailsThe callback signature now matches Base UI:
Existing code that only uses the first argument continues to work unchanged.
Removed deprecated props
onChangeonCheckedChangeonValueChangeonCheckedChangeonClickSimplified internals
handleCheckedChangewrapper functions in bothCheckboxBaseandCheckboxItemonCheckedChangedirectly to Base UIBaseCheckbox.Root.Props["onCheckedChange"]New export
Added
CheckboxChangeEventDetailstype export for consumers who need to type the event details:What's NOT changing
Checkbox.Group'sonValueChangeprop - This is a different API that returnsstring[](the array of selected values), not a boolean. It remains unchanged.Table.CheckCellandTable.CheckHead- These have their ownonValueChangeprop that wraps Checkbox internally. They continue to provide a simpler(checked: boolean) => voidAPI, which makes sense for table selection use cases.Migration
Breaking Change Justification
onChangeprop was usingas nevercast internally, which was a code smell