Skip to content
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

Merged
merged 2 commits into from Feb 19, 2021

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Feb 18, 2021

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 with state.value and setting state.value to props.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)

@junlincc junlincc added the explore:control Related to the controls panel of Explore label Feb 18, 2021
@junlincc
Copy link
Member

tested locally, LGTM! ✅ Thank you for the fix!

@junlincc junlincc assigned villebro and unassigned villebro Feb 18, 2021
Copy link
Member

@villebro villebro left a 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.

@villebro villebro merged commit 42ff4fc into apache:master Feb 19, 2021
@ktmud
Copy link
Member

ktmud commented Feb 19, 2021

Also related #11985

@ktmud
Copy link
Member

ktmud commented Feb 19, 2021

Hi @junlincc @pkdotson @villebro @kgabryje , going back to the original PR (#12782 ) that introduced this bug, I don't think TextControl should be included in the controls that reset values when users change datasource. Because most of the time, the input value in TextControl is not datasource specific---i.e. they are not related to columns and metrics. As discussed in #12782, the original input (axis labels, bounds, etc.) could be useful when plotting with a new datasource.

I'm going to remove the reset logic in my control refactoring PR if you don't mind.

@junlincc
Copy link
Member

@ktmud remove it all or just text input?

@ktmud
Copy link
Member

ktmud commented Feb 20, 2021

@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.

@junlincc
Copy link
Member

@ktmud
i am curious when users change dataset, do they also change chart type as well?
do users create new chart by clicking +button, or from the explore view by changing dataset and chart type.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels explore:control Related to the controls panel of Explore size/S 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants