Add v-model support to KCheckbox #1140

WinnyChang · 2025-10-13T00:49:22Z

Description

Add v-model support to KCheckbox.
Update the KCheckbox documentation page.
Update unit tests and visual tests to work with KCheckbox's v-model. (Percy build for KCheckbox visual tests)

Issue addressed

Fixes #1135

Changelog

Description: Add v-model support to KCheckbox; update documentation page, unit tests, and visual tests.
Products impact: none
Addresses: Add v-model support to KCheckbox #1135
Components: -
Breaking: -
Impacts a11y: -
Guidance: -

Testing checklist

Contributor has fully tested the PR manually
Critical and brittle code paths are covered by unit tests
The change is described in the changelog section above

Reviewer guidance

Is the code clean and well-commented?
There are unit tests, and they use @testing-library/vue.
There are visual tests.
v-model is documented on KCheckbox documentation page, including new examples.

MisRob · 2025-10-15T11:51:58Z

Hi @WinnyChang, good to see you around and thanks a lot :) I will review documentation. For the feature itself, we will assign a reviewer next week.

WinnyChang · 2025-10-15T18:22:20Z

@MisRob Got it. Thanks for the update!

AlexVelezLl

Thanks a lot, @WinnyChang, this looks pretty good! However, this change was not intended to be a breaking change, i.e,. KDS consumers should not need to migrate all existing checkboxes to the new v-model api after this change is released; it should be an additional, optional api. So both APIs should be supported by KCheckbox: the v-model one and the :checked/@change API. Apart from that, this looks good!

docs/pages/kcheckbox.vue

lib/KCheckbox/index.vue

AlexVelezLl · 2025-10-24T16:35:39Z

lib/KCheckbox/index.vue

      /**
-       * Checked state
+       * Reactive v-model checkbox state. Updates with the `input` event.
       */


Are these double comment blocks the intended result to only show the first one in the docs page?

Yes, that’s intentional — only the first comment block is shown. The hidden ones are copied from Studio's Checkbox.

I chose to show only the first ones since the comments are quite long, but I can include all of them in the docs page if you think that would be better.

lib/KCheckbox/index.vue

MisRob · 2025-10-24T16:50:43Z

this change was not intended to be a breaking change, i.e,. KDS consumers should not need to migrate all existing checkboxes to the new v-model api after this change is released

Ah yes, I should have mentioned it explicitly in the issue - apologies @WinnyChang @AlexVelezLl.

Preventing a breaking change is an important aspect, and another reason is that even though v-model will be likely most commonly used, in some more nuanced cases, we may still need :checked/@change.

As for the documentation, I would suggest that the above is reflected:

All examples updated to use to v-model since we want to encourage it whenever possible
A new section that demonstrates how to use :checked/@change so developers are aware there's that possibility and how to use it

…s' description, add aditional unit tests

MisRob · 2025-10-28T12:26:43Z

Just leaving a note that I skimmed quickly through the updated documentation and it felt clear to me, and as balanced amount of information. Very nice work, thank you.

github-actions · 2025-10-28T15:18:52Z

Percy Visual Test Results

Percy Dashboard: View Detailed Report

Environment:

Node.js Version: 18.x
OS: Ubuntu-latest

Instructions for Reviewers:

Click on the Percy Dashboard link to view detailed visual diffs.
Review the visual changes highlighted in the report.
Approve or request changes based on the visual differences.

AlexVelezLl

Thanks @WinnyChang! We're almost there! Just found a little regression in Kolibri because of an unrecognized legacy api usage, and reflected on a possible way this could be implemented safely.

AlexVelezLl · 2025-10-28T18:27:52Z

lib/KCheckbox/index.vue

+      usingLegacyApi() {
+        return this.checked !== undefined;
+      },


Just to be extra cautious, the old API assumed that if checked was null or undefined, the checkbox was unchecked, and there may be some consumers who rely on this too. So, to determine if the legacy api is being used, I think we can instead assert against the new value or inputValue, and if any of them are set, then we are not using the legacy API (which would 100% cover all examples of KCheckbox right now).

From a brief skim, I found a place in Kolibri where we assume that undefined is the falsy value of the checkbox, and this PR is introducing a regression on this checkbox:

Grabacion.de.pantalla.2025-10-28.a.la.s.1.34.14.p.m.mov

AlexVelezLl · 2025-10-28T18:32:13Z

lib/KCheckbox/index.vue

+          this.isChecked = !this.isChecked;
+          if (this.usingLegacyApi) {
+            /**
+             * Emits change event with the new legacy API checkbox state.
+             */
+            this.$emit('change', !this.checked, event);
+          }
        }
      },
+      updateInputValue(newValue) {
+        /**
+         * Emits input event with the new v-model checkbox state.
+         * Used by v-model to keep the checkbox state in sync.
+         */
+        this.$emit('input', newValue);
+      },


I think we can centralize the emission of the input/change event in the updateInputValue and let it decide which event to emit. This way, we abstract the change of the isChecked computed property. If, for any reason, another method changes its value, the different methods should not have to determine whether to emit or not the change event. (Not sure if I made this clear, if not please let me know 😅)

The other option may be to use the @change event as the input event for the v-model instead of @input, and just always emit this event for all values independently of the api that is being used. I think that may be the safest way to prevent any unintended behavior if, for any reason, a consumer sets an undefined value as a falsy value, but they intended to use the v-model api. What do you think @WinnyChang, @MisRob?

This will prevent any confusion if we ever, for any reason (not reading the documentation well 😅), declare something like:

<template> <KCheckbox v-model="checkboxValue" ... /> </template> <script> ... setup() { return { checkboxValue: ref(), } } </script>

This would fail if we determine that this is not using the new api because both their value and input value are undefined, then we won't emit the input event, and would emit the change event instead. But if we used the same event for both, we wouldn't need to worry about cases like this.

I think we can centralize the emission of the input/change event in the updateInputValue and let it decide which event to emit.

In my current code, I separate the mechanism of the legacy API and v-model, so the legacy API won't trigger updateInputValue. Is it okay to trigger updateInputValue when using the legacy API and put the input/change emissions together?

Yes! That would be ideal! It should be the responsibility of the computed property to determine how to manage the changes to the isChecked property, in any case. This helps us centralize the property update mechanism. If, for some reason, we want to change the value of this variable in another method (without using the current toggle method) in the future, the legacy update mechanism will still work without duplicating code!

The other option may be to use the @change event as the input event for the v-model instead of @input, and just always emit this event for all values independently of the api that is being used.

This sounds like a good idea. The only concern I have is that @change is with a boolean, while @input is with array/boolean/number/string/object. I wonder if this will cause any issues in some cases.

I also thought about always emitting both events, no matter which API is used. But I wasn't sure if it follows the best practices. 😂

Just coordinated with the team, and yes, we agreed on having the change event as the event for the v-model api @WinnyChang 😄. So we would just always emit the @change event

Got it! I'll update accordingly.

MisRob · 2025-11-13T07:26:45Z

Thanks @WinnyChang - we will re-review. We have more internal commitments this and next week so it may take longer than usual.

AlexVelezLl

Just some final nitpicks! Everything else looks excellent! Thanks @WinnyChang!!

AlexVelezLl · 2025-11-20T14:17:37Z

lib/KCheckbox/index.vue

+          if (typeof this.inputValue === 'boolean') {
+            return this.inputValue;
+          }
+          return Boolean(this.inputValue !== null);


(Nitpick) Since this.inputValue !== null is already a comparison, it returns a boolean value, and we no longer need to wrap it with the Boolean() type

AlexVelezLl · 2025-11-20T14:35:45Z

lib/KCheckbox/index.vue

-           */
-          this.$emit('change', !this.checked, event);
+          this.isChecked = !this.isChecked;
+          this.lastUserEvent = null;


At first read, it is not apparent what the lastUserEvent is for here, or why it is set to null again. We should add a couple of comments to explain this better.

However, to play safe, I would suggest setting the lastUserEvent to null inside a nextTick call, just to make sure that this event is being set to null just after the change event has already been emitted, and not before. Another alternative to actually make sure that this is set to null just after the change event has been emitted is to set it to null within the updateInputValue method. Since it would mean "okay, I already used it, we don't need this anymore", and this will ensure that this value is removed just after the change event has been emitted.

WinnyChang added 3 commits October 6, 2025 17:42

Add v-model support to KCheckbox and update its documentation page

d10ef25

Update existing unit tests to use VTL and to work with v-model

eaaa215

Update existing visual tests to work with v-model

606fead

WinnyChang had a problem deploying to percy_tests October 13, 2025 00:49 — with GitHub Actions Failure

MisRob added the TODO: needs review Waiting for review label Oct 15, 2025

AlexVelezLl self-assigned this Oct 20, 2025

AlexVelezLl requested changes Oct 24, 2025

View reviewed changes

MisRob added the hacktoberfest-accepted label Oct 24, 2025

WinnyChang added 2 commits October 26, 2025 19:46

Support both :checked/@change and v-model API, adjust props and event…

3a78465

…s' description, add aditional unit tests

Use DocsExample and example files, add :checked/@change examples

23f6167

WinnyChang temporarily deployed to percy_tests October 27, 2025 06:45 — with GitHub Actions Inactive

AlexVelezLl requested changes Oct 28, 2025

View reviewed changes

Update to use the change event for both v-model and legacy API

4ccac4d

WinnyChang temporarily deployed to percy_tests November 7, 2025 05:12 — with GitHub Actions Inactive

MisRob requested a review from AlexVelezLl November 13, 2025 07:24

AlexVelezLl reviewed Nov 20, 2025

View reviewed changes

Add v-model support to KCheckbox #1140

Are you sure you want to change the base?

Add v-model support to KCheckbox #1140

Uh oh!

Conversation

WinnyChang commented Oct 13, 2025 • edited by MisRob Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue addressed

Changelog

Testing checklist

Reviewer guidance

Uh oh!

MisRob commented Oct 15, 2025

Uh oh!

WinnyChang commented Oct 15, 2025

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MisRob commented Oct 24, 2025

Uh oh!

MisRob commented Oct 28, 2025

Uh oh!

github-actions bot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Percy Visual Test Results

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MisRob commented Nov 13, 2025

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

WinnyChang commented Oct 13, 2025 •

edited by MisRob

Loading

github-actions bot commented Oct 28, 2025 •

edited

Loading