fix(lit): update Slider display value on user input#1022
fix(lit): update Slider display value on user input#1022ppazosp wants to merge 1 commit intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request removes an unused import and adds a call to requestUpdate in the slider component to ensure the UI updates correctly after data changes. A reviewer suggested encapsulating the data update and UI refresh logic into a helper method within the base class to improve maintainability and consistency across components. Additionally, the changes lack accompanying tests, which is a violation of the repository's style guide.
renderers/lit/src/0.8/ui/slider.ts
Outdated
| value, | ||
| this.surfaceId ?? A2uiMessageProcessor.DEFAULT_SURFACE_ID | ||
| ); | ||
| this.requestUpdate(); |
There was a problem hiding this comment.
While this fixes the UI update issue, the pattern of calling setData followed by requestUpdate could be encapsulated in a helper method to improve maintainability and ensure consistency, especially since you've noted other components will need a similar fix.
Consider adding a helper method to the Root base class in renderers/lit/src/0.8/ui/root.ts:
// In Root class
protected updateData(relativePath: string, value: DataValue) {
if (!this.processor) {
return;
}
this.processor.setData(
this.component,
relativePath,
value,
this.surfaceId ?? A2uiMessageProcessor.DEFAULT_SURFACE_ID
);
this.requestUpdate();
}The Slider's #setBoundValue method could then be refactored to use it (though this part is outside the current diff):
#setBoundValue(value: string) {
if (!this.value || !("path" in this.value) || !this.value.path) {
return;
}
this.updateData(this.value.path, value);
}This would make the pattern reusable for fixing the other components you mentioned in the follow-up work. What do you think?
There was a problem hiding this comment.
Great suggestion! I added updateBoundData() as a protected method on the Root base class and refactored the Slider to use it. This makes it easy to apply the same fix to other interactive components (TextField, DateTimeInput, CheckBox) in follow-up PRs.
3d788c9 to
4076286
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared updateBoundData method in the Root component to centralize data updates and refactors the Slider component to utilize this method. Feedback suggests improving the readability of the path validation in the Slider component by extracting the path into a variable and notes the absence of required tests for the changes as per the style guide.
| if (!this.value || !("path" in this.value) || !this.value.path) { | ||
| return; | ||
| } | ||
|
|
||
| if (!("path" in this.value)) { | ||
| return; | ||
| } | ||
|
|
||
| if (!this.value.path) { | ||
| return; | ||
| } | ||
|
|
||
| this.processor.setData( | ||
| this.component, | ||
| this.value.path, | ||
| value, | ||
| this.surfaceId ?? A2uiMessageProcessor.DEFAULT_SURFACE_ID | ||
| ); | ||
| this.updateBoundData(this.value.path, value); |
There was a problem hiding this comment.
For improved readability, you could consider extracting the path into a variable first, then checking its validity. This would make the intent of the code clearer at a glance.
const path = this.value && "path" in this.value ? this.value.path : undefined;
if (!path) {
return;
}
this.updateBoundData(path, value);There was a problem hiding this comment.
I considered this, but the current pattern (guard clause with early return) is consistent with how all other interactive components handle path validation (checkbox.ts, datetime-input.ts, text-field.ts). Changing it here would make the Slider inconsistent with the rest. I'd rather keep it aligned.
| #setBoundValue(value: string) { | ||
| if (!this.value || !this.processor) { | ||
| if (!this.value || !("path" in this.value) || !this.value.path) { | ||
| return; | ||
| } | ||
|
|
||
| if (!("path" in this.value)) { | ||
| return; | ||
| } | ||
|
|
||
| if (!this.value.path) { | ||
| return; | ||
| } | ||
|
|
||
| this.processor.setData( | ||
| this.component, | ||
| this.value.path, | ||
| value, | ||
| this.surfaceId ?? A2uiMessageProcessor.DEFAULT_SURFACE_ID | ||
| ); | ||
| this.updateBoundData(this.value.path, value); | ||
| } |
There was a problem hiding this comment.
This is a great fix for the slider value update issue. However, I noticed that there are no new tests included with this change. The repository style guide requires tests for code changes. Adding a regression test for this bug would be valuable to ensure it doesn't reappear in the future. Could you please add a test that covers this scenario?
References
- If there are code changes, code should have tests. (link)
There was a problem hiding this comment.
There's no existing test infrastructure for v0.8 UI components in the Lit renderer, renderers/lit/ only has model.test.ts and v0.9 tests. Setting up component testing (DOM rendering, event simulation) for v0.8 would be a significant effort beyond the scope of this one-line fix. Happy to contribute tests as a follow-up if the maintainers set up the testing scaffold or point me to the preferred approach.
Add `updateBoundData()` helper to Root base class that encapsulates the setData + requestUpdate pattern. Refactor Slider to use it. Closes google#597
4076286 to
6a14f08
Compare
Description
The Lit v0.8 Slider component does not update its displayed value when the user drags the slider. The
<span>showing the current numeric value remains stuck at the initial value.Root cause:
#setBoundValuecallsprocessor.setData()to update the data model, but the component never re-renders because the signal change fromSignalMap.set()is not being picked up bySignalWatcher. As a result,render()is never called again after the initial render, andextractNumberValue()in the<span>keeps showing the stale value.Fix: Add a
updateBoundData()helper method to theRootbase class that encapsulates thesetData()+requestUpdate()pattern. Refactored the Slider's#setBoundValueto use it. This is consistent with the existing pattern in theMultipleChoicecomponent (multiple-choice.ts:308), which already callsrequestUpdate()after updating the data model.Also removed an unused
Typesimport from the Slider.Closes #597
Notes
Other interactive components (
TextField,DateTimeInput,CheckBox) use the samesetData()-without-requestUpdate()pattern and likely have the same latent bug. It is less visible inTextFieldbecause the native<input>element displays typed characters regardless of Lit re-renders. These components can now be easily refactored to useupdateBoundData()in follow-up PRs.Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.