feat(Checkbox/Switch): add support for trueValue / falseValue props#6150
Merged
benjamincanac merged 13 commits intov4from Mar 8, 2026
Merged
feat(Checkbox/Switch): add support for trueValue / falseValue props#6150benjamincanac merged 13 commits intov4from
trueValue / falseValue props#6150benjamincanac merged 13 commits intov4from
Conversation
commit: |
sandros94
approved these changes
Mar 6, 2026
…ith other components
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR makes Checkbox and Switch generic (script setup generic="T = boolean"), adding typed Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/runtime/components/Switch.vue (1)
99-105: Synthetic event'stargetproperty won't be set at runtime.The
Eventconstructor ignores thetargetproperty inEventInit—event.targetwill benulluntil the event is dispatched on a DOM element. If any consumer of thechangeevent expectsevent.target.value, it will fail silently.If the value needs to be accessible, consider using a
CustomEventwith the value indetail:💡 Optional improvement
function onUpdate(value: any) { - // `@ts-expect-error` - 'target' does not exist in type 'EventInit' - const event = new Event('change', { target: { value } }) + const event = new CustomEvent('change', { detail: { value } }) emits('change', event) emitFormChange() emitFormInput() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Switch.vue` around lines 99 - 105, The synthetic Event created in onUpdate (in Switch.vue) sets a non-existent target and will have event.target === null at runtime, so replace the Event with a CustomEvent that carries the new value in detail (e.g. new CustomEvent('change', { detail: { value } })), then call emits('change', customEvent) and keep the existing emitFormChange() and emitFormInput() calls; ensure any consumers expecting event.target.value are updated to read event.detail.value instead or emit a plain value alongside the event if you prefer backward compatibility.src/runtime/components/Checkbox.vue (1)
103-109: Same synthetic event limitation as Switch.vue.This has the same
Eventconstructor limitation wheretargetwon't be set. Consider applying the same fix if you decide to useCustomEventin Switch.vue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Checkbox.vue` around lines 103 - 109, The synthetic Event created in onUpdate does not support a custom target property; replace the Event with a CustomEvent (e.g., new CustomEvent('change', { detail: { value }, bubbles: true })) and update callers to read the value from event.detail.value (or attach a small wrapper object if needed) before calling emits('change', event), then call emitFormChange() and emitFormInput() as before; update any downstream consumers expecting event.target.value to use event.detail.value (or adapt the wrapper) so onUpdate, emits('change', emitFormChange, and emitFormInput remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/components/Checkbox.vue`:
- Around line 103-109: The synthetic Event created in onUpdate does not support
a custom target property; replace the Event with a CustomEvent (e.g., new
CustomEvent('change', { detail: { value }, bubbles: true })) and update callers
to read the value from event.detail.value (or attach a small wrapper object if
needed) before calling emits('change', event), then call emitFormChange() and
emitFormInput() as before; update any downstream consumers expecting
event.target.value to use event.detail.value (or adapt the wrapper) so onUpdate,
emits('change', emitFormChange, and emitFormInput remain consistent.
In `@src/runtime/components/Switch.vue`:
- Around line 99-105: The synthetic Event created in onUpdate (in Switch.vue)
sets a non-existent target and will have event.target === null at runtime, so
replace the Event with a CustomEvent that carries the new value in detail (e.g.
new CustomEvent('change', { detail: { value } })), then call emits('change',
customEvent) and keep the existing emitFormChange() and emitFormInput() calls;
ensure any consumers expecting event.target.value are updated to read
event.detail.value instead or emit a plain value alongside the event if you
prefer backward compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efe0cb2d-bd69-4098-9195-4ced7966c5a2
📒 Files selected for processing (3)
src/runtime/components/Checkbox.vuesrc/runtime/components/Switch.vuetest/components/Table.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Checkbox.vue`:
- Line 12: The prop typing for CheckboxProps currently picks 'modelValue' from
CheckboxRootProps but must use the normalized tri-state 'state' instead to
correctly detect indeterminate when users supply custom trueValue/falseValue;
update the Pick in CheckboxProps to include 'state' in place of 'modelValue'
(retain the other keys: 'disabled', 'required', 'name', 'value', 'id',
'defaultValue', 'trueValue', 'falseValue'), and ensure any usages of
CheckboxProps that relied on modelValue for indeterminate checks switch to the
CheckboxRoot slot's state ('false' | true | 'indeterminate') for tri-state
logic.
- Around line 53-55: The change event currently emits a plain Event that cannot
carry typed values for the generic model (CheckboxEmits -> change), so replace
the use of Event with a CustomEvent carrying the new value in its detail (e.g.,
CustomEvent<T | 'indeterminate'>) and update CheckboxEmits to declare change:
[event: CustomEvent<T | 'indeterminate'>]; ensure the component dispatches new
CustomEvent(...) with the toggled value in detail instead of attempting to write
to event.target, and propagate the same pattern to other form components that
currently use new Event('change', { target: { value } }) so consumers can read
typed payloads from event.detail.
In `@src/runtime/components/Switch.vue`:
- Around line 50-52: The `change` emit currently declares an Event payload but
synthetic Events can't carry a custom target/value; update SwitchEmits<T> so the
change event exposes the switch value directly (e.g., change: [value: T]) and
adjust emit sites in the component (where emit('change', ...), or any emitChange
helper) to pass the new value (or emit a CustomEvent with the value in detail)
instead of trying to construct an Event with a target. Also update any
listeners/tests/types expecting Event to use the new value payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a341762-2472-403b-9728-97058747b9de
📒 Files selected for processing (2)
src/runtime/components/Checkbox.vuesrc/runtime/components/Switch.vue
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/components/Checkbox.vue (1)
53-55:⚠️ Potential issue | 🟠 Major
changestill drops the generic payload.With
trueValue/falseValue, consumers may need the newTfromchange, butEventcannot carry it here—EventInithas notargetpayload. If this event is meant to expose the toggled value, type it asCustomEvent<T | 'indeterminate'>and emit the value viadetailinstead.Minimal shape
-export interface CheckboxEmits<T = boolean> extends CheckboxRootEmits<T> { - change: [event: Event] +export interface CheckboxEmits<T = boolean> extends CheckboxRootEmits<T> { + change: [event: CustomEvent<T | 'indeterminate'>] }function onUpdate(value: T | 'indeterminate') { emits('change', new CustomEvent('change', { detail: value })) emitFormChange() emitFormInput() }Does the DOM `Event` constructor accept a `target` field in `EventInit`, and is `CustomEvent.detail` the standard way to attach a typed payload?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Checkbox.vue` around lines 53 - 55, The current CheckboxEmits<T> defines change as Event which loses the generic payload; change should be typed as CustomEvent<T | 'indeterminate'> so consumers receive the toggled value via detail. Update the interface entry `change: [event: CustomEvent<T | 'indeterminate'>]` and adjust the emitter call (e.g., in the `onUpdate`/emit site) to emit `new CustomEvent('change', { detail: value })` rather than a plain Event so the typed payload is carried in `detail`. Ensure any call sites and tests expecting Event are updated to read event.detail to access the T or 'indeterminate' value.
🧹 Nitpick comments (1)
test/components/Checkbox.spec.ts (1)
71-96: Exercise the real toggle path here.These assertions only prove that
UCheckboxre-emits whateverCheckboxRootalready emitted. They would still pass if a click started emittingtrue/falseagain, or if unchecking stopped returningfalseValue. Please add one interaction-based case that toggles both directions and asserts'yes'/'no'or1/0end to end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/Checkbox.spec.ts` around lines 71 - 96, Add an interaction-based test that actually toggles the checkbox UI twice and asserts end-to-end values instead of just re-emitting: mount Checkbox with custom trueValue/falseValue and defaultValue, locate the real clickable element (the CheckboxRoot component or its input), trigger a user interaction (e.g., element.trigger('click') or wrapper.find('input').trigger('click')) to check it and assert the emitted 'update:modelValue' is the custom trueValue (and emitted change event), then trigger a second click to uncheck and assert 'update:modelValue' is the custom falseValue (and emitted change event); update the test suite alongside the existing tests that reference Checkbox and CheckboxRoot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/runtime/components/Checkbox.vue`:
- Around line 53-55: The current CheckboxEmits<T> defines change as Event which
loses the generic payload; change should be typed as CustomEvent<T |
'indeterminate'> so consumers receive the toggled value via detail. Update the
interface entry `change: [event: CustomEvent<T | 'indeterminate'>]` and adjust
the emitter call (e.g., in the `onUpdate`/emit site) to emit `new
CustomEvent('change', { detail: value })` rather than a plain Event so the typed
payload is carried in `detail`. Ensure any call sites and tests expecting Event
are updated to read event.detail to access the T or 'indeterminate' value.
---
Nitpick comments:
In `@test/components/Checkbox.spec.ts`:
- Around line 71-96: Add an interaction-based test that actually toggles the
checkbox UI twice and asserts end-to-end values instead of just re-emitting:
mount Checkbox with custom trueValue/falseValue and defaultValue, locate the
real clickable element (the CheckboxRoot component or its input), trigger a user
interaction (e.g., element.trigger('click') or
wrapper.find('input').trigger('click')) to check it and assert the emitted
'update:modelValue' is the custom trueValue (and emitted change event), then
trigger a second click to uncheck and assert 'update:modelValue' is the custom
falseValue (and emitted change event); update the test suite alongside the
existing tests that reference Checkbox and CheckboxRoot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 073ccf13-7658-4aae-8fe9-8c8763f1e97d
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Checkbox-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Checkbox.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/runtime/components/Checkbox.vuetest/components/Checkbox.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/components/Switch.spec.ts`:
- Around line 70-95: The tests currently emit directly from SwitchRoot which
bypasses prop forwarding; update the three tests for Switch to assert that the
SwitchRoot component receives the trueValue/falseValue props (e.g., check
wrapper.findComponent({ name: 'SwitchRoot' }).props() includes
trueValue/falseValue) and in the "change event with custom trueValue/falseValue"
test also assert that the emitted change event payload includes the custom value
on event.target.value (i.e., after emitting update:modelValue use
wrapper.emitted('change') to verify the object contains target.value === the
custom trueValue/falseValue); ensure you reference the Switch component, the
SwitchRoot child, and the trueValue/falseValue props when adding these
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d44c60e9-9cdf-42f0-97ec-4798a2977e73
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Switch-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Switch.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
test/components/Switch.spec.ts
…interaction tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
Resolves #5821, follows-up #6147
❓ Type of change
📚 Description
📝 Checklist