-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
fix(explore): Reset values in TextControl only when datasource changes #13211
Conversation
tested locally, LGTM! ✅ Thank you for the fix! |
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.
LGTM, thanks for fixing 👍 The fact that a few simple and seemingly unrelated changes caused a regression like this makes me think we'd need to rethink how these components are structured.
Also related #11985 |
Hi @junlincc @pkdotson @villebro @kgabryje , going back to the original PR (#12782 ) that introduced this bug, I don't think I'm going to remove the reset logic in my control refactoring PR if you don't mind. |
@ktmud remove it all or just text input? |
@junlincc Just the text control. As discussed in #12782 (comment) , the reset logic should be only applied to controls that depend on datasource columns/metrics. |
@ktmud |
SUMMARY
Due to recent changes in #12782 regarding resetting control values when datasource changes, debounce in TextControl component stopped working properly. This was caused by comparing
props.value
withstate.value
and settingstate.value
toprops.value
if they were not equal. Because of using 0.5s debounce, props and state values were always unequal when user typed a new value. This PR fixes the issue by adding a comparison of props and state datasource and updating value only when datasources are not equal.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: see #13191
After:
https://user-images.githubusercontent.com/15073128/108376726-8ebab080-7203-11eb-8ef2-f3ef73aab2f3.mov
TEST PLAN
ADDITIONAL INFORMATION
CC: @villebro @junlincc
@nikolagigic @mayurnewase please help review this PR, thanks! (from junlin)