-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix CheckboxList UI not updating when values are set programmatically #19438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v15/dev
Are you sure you want to change the base?
Fix CheckboxList UI not updating when values are set programmatically #19438
Conversation
Hi there @readingdancer, thank you for this contribution! 👍 While we wait for one of the Core Collaborators 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Can you try again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a UI synchronization bug in the CheckboxList property editor that prevented programmatically set values from immediately updating the UI.
- Added a private method #updateCheckedState() to update the checked property on list items based on the current selection.
- Modified the value setter to call #updateCheckedState(), ensuring the UI is correctly re-rendered when values change.
Comments suppressed due to low confidence (1)
src/Umbraco.Web.UI.Client/src/packages/property-editors/checkbox-list/property-editor-ui-checkbox-list.element.ts:33
- Ensure that the new #updateCheckedState method is thoroughly covered by tests, especially for edge cases like handling an empty _list or invalid values in selection.
this.#updateCheckedState();
As requested by Copilot, here are some unit tests to ensure this addition passes all of the possible edge cases mentioned.
@leekelleher - Thanks for triggering that CoPilot review, based on it's feedback I've added some unit tests. I hope someone can take a look at this PR as I need the fix for a package I am building :) I have another similar PR in the pipeline for dropdowns and multi-select boxes, but it would be good to know if these will get approved / needs any changes before I submit the next one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Fix CheckboxList UI not updating when values are set programmatically
- Added
#updateCheckedState()
to synchronize visual checkbox states when thevalue
setter is called - Updated the
value
setter to invoke the new method and trigger a Lit re-render - Expanded tests to cover programmatic value setting, various configurations, and readonly mode
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/Umbraco.Web.UI.Client/src/packages/property-editors/checkbox-list/property-editor-ui-checkbox-list.element.ts | Added #updateCheckedState() and wired it into the value setter |
src/Umbraco.Web.UI.Client/src/packages/property-editors/checkbox-list/property-editor-ui-checkbox-list.test.ts | Added tests for programmatic value setting, configuration handling, and readonly behavior |
Comments suppressed due to low confidence (2)
src/Umbraco.Web.UI.Client/src/packages/property-editors/checkbox-list/property-editor-ui-checkbox-list.element.ts:111
- The
readonly
property onUmbPropertyEditorUICheckboxListElement
isn’t propagated to the child<umb-input-checkbox-list>
, so the UI won’t actually render in readonly mode. Bind thereadonly
attribute (or property) through to the child component.
<umb-input-checkbox-list
src/Umbraco.Web.UI.Client/src/packages/property-editors/checkbox-list/property-editor-ui-checkbox-list.test.ts:42
- Tests currently assert only the
selection
array. Add assertions that inspect the actual rendered checkbox inputs (e.g.,input[type=checkbox]
) to confirm theirchecked
properties match the expected state.
const checkboxListInput = element.shadowRoot?.querySelector('umb-input-checkbox-list');
...ient/src/packages/property-editors/checkbox-list/property-editor-ui-checkbox-list.element.ts
Outdated
Show resolved
Hide resolved
Removed a check that was redundant and removed a unit test that was also not needed for the current PR and fixed one of the other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a UI synchronization bug in the CheckboxList property editor so that the checkbox states update immediately when values are set programmatically. The key changes include:
- Invoking a new method (#updateCheckedState) in the value setter to keep the internal selection and visual state in sync.
- Adding comprehensive test cases to validate programmatic updates, configuration handling, and edge case behavior.
- Improving overall test coverage with helper functions to verify the DOM and model state correlation.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
property-editor-ui-checkbox-list.element.ts | Introduced #updateCheckedState to update checkbox states immediately upon value changes |
property-editor-ui-checkbox-list.test.ts | Added tests to validate UI updates for various programmatic value settings and configuration scenarios |
How to replicate the issue and test that the PR fixes it.You can easily test this in a browser.
What we expect is this will select the box, but nothing seems to happen, even though you have set the correct variable. Note: In our plugin code we would be using setValue to do the same thing. Now if you use my PR and try the same thing, you will see it does select the boxes. |
Fixes: #19437
Prerequisites
This fixes the CheckboxList UI update issue where programmatically set values don't reflect in the UI until page refresh.
Description
What did I do?
Fixed a UI synchronization bug in the CheckboxList property editor where setting values programmatically (via JavaScript) would not update the visual state of the checkboxes until the page was refreshed.
Why did I do it?
The UmbPropertyEditorUICheckboxListElement had a disconnect between its internal value storage (#selection) and the UI rendering state (_list items). When the value setter was called programmatically, it updated the selection array but failed to sync the checked property of the list items that control the visual checkbox states.
This caused confusion for developers building custom property editors or integrations, as the UI would appear broken despite the underlying data being correct.
How can we test the changes?
Technical Implementation:
Code Changes:
This fix ensures the CheckboxList property editor behaves consistently whether values are set through user interaction or programmatically, improving the developer experience for custom integrations and property editors.