Skip to content

fix(choice-group): handle boolean values in multipleChoice checkbox groups#2672

Open
gouthamx67 wants to merge 2 commits intoing-bank:masterfrom
gouthamx67:fix-choice-group-multiplechoice
Open

fix(choice-group): handle boolean values in multipleChoice checkbox groups#2672
gouthamx67 wants to merge 2 commits intoing-bank:masterfrom
gouthamx67:fix-choice-group-multiplechoice

Conversation

@gouthamx67
Copy link

What was done

In the _setCheckedElements method of ChoiceGroupMixin.js, a small safeguard was added to handle cases where multipleChoice is enabled but the incoming modelValue is not an array.

In some situations (such as when navigating back to a page), modelValue could be a boolean. Since the existing logic assumes an array and calls .includes(), this caused a runtime error.

To fix this, the value is now first normalized:

If it’s already an array, it’s used as-is

If it’s not an array, it’s treated as an empty array

Both .includes() checks were updated to use this normalized value, ensuring the logic remains safe and predictable.

@changeset-bot
Copy link

changeset-bot bot commented Jan 14, 2026

🦋 Changeset detected

Latest commit: 842c9fd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lion/ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@CLAassistant
Copy link

CLAassistant commented Jan 14, 2026

CLA assistant check
All committers have signed the CLA.

@okadurin
Copy link
Collaborator

Hi @gouthamx67 Thank you for contribution! May I ask you to revert all the unrelated changes like those outside of packages/ui/components/form-core/src/choice-group/ChoiceGroupMixin.js file. If an empty array is the problem you could move // @ts-nocheck just above the line with the empty array (not at the beginning of the file). Also could you please run npm run changeset to generate a changelog for a patch version of @lion/ui library

@gouthamx67
Copy link
Author

Hi! Thanks for the review and the clear guidance 🙏

I’ve reverted all unrelated changes and limited the update to packages/ui/components/form-core/src/choice-group/ChoiceGroupMixin.js. I also moved // @ts-nocheck to just above the empty array as suggested, and ran npm run changeset to generate a patch changelog for @lion/ui.

I’m new to open source, so apologies for any inconvenience caused earlier , really appreciate the feedback and patience. Please let me know if anything else needs adjustment!

@okadurin
Copy link
Collaborator

  • When I open a Files changed tab I would expect to see only packages/ui/components/form-core/src/choice-group/ChoiceGroupMixin.js and .changeset/shy-buttons-sing.md. Please revert other files.
  • Also you need to sing the Contributor License Agreement in the PR

@gouthamx67 gouthamx67 force-pushed the fix-choice-group-multiplechoice branch from 6f450d1 to 19cce90 Compare January 16, 2026 10:42
@gouthamx67
Copy link
Author

Thanks for your patience and guidance — still learning my way around Git and PR hygiene, and this was a great learning experience for me. I’ve cleaned up the PR so only the intended files are changed and signed the CLA. Please let me know if anything else is needed

@okadurin
Copy link
Collaborator

okadurin commented Jan 16, 2026

  1. Please fix the lintin issues. You could run npm run format:prettier and commit only files you changed.
  2. There are failing tests in the pipeline. If you open some apparently the current source code allows setting a string as a value for multipleChoice case. Check this test as an example: https://github.com/ing-bank/lion/blob/master/packages/ui/components/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js#L242 .
    It means checking if the value is Array only is too strict condition. We also need to allow strings.
  3. Instead of using //ts-nocheck could you use the typing as we had before: /** @type {any[]} */ (value)

@gouthamx67
Copy link
Author

hey , Ive updated the logic to allow single string values for multipleChoice and fixed the linting/type issues as requested :)

@okadurin
Copy link
Collaborator

hey , Ive updated the logic to allow single string values for multipleChoice and fixed the linting/type issues as requested :)

  • Please make sure the PR contains only the changes related to your PR. Revert all unrelated changes
  • Could you add a test for this case please? The test should fail for master branch and it should pass with your branch changes
    • To run the tests you could run npm run debug and by hitting F or D after you could focus on a particular test file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants