You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/18 14:50:59 UTC
[GitHub] [superset] kgabryje opened a new pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
kgabryje opened a new pull request #13211:
URL: https://github.com/apache/superset/pull/13211
### SUMMARY
Due to recent changes regarding resetting control values when datasource changes, debounce in TextControl component did not work 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 https://github.com/apache/superset/issues/13191
After:
Uploading Nagranie z ekranu 2021-02-18 o 15.49.45.mov…
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: fixes https://github.com/apache/superset/issues/13191
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
CC: @villebro @junlincc @nikolagigic @mayurnewase
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-782567858
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #13211:
URL: https://github.com/apache/superset/pull/13211
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-782416788
Also related #11985
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-782550546
@ktmud remove it all or just text input?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-782554932
@junlincc Just the text control. As discussed in https://github.com/apache/superset/pull/12782#issuecomment-770119750 , the reset logic should be only applied to controls that depend on datasource columns/metrics.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-782418459
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.
I'm going to remove the reset logic in my [control refactoring PR](https://github.com/apache/superset/pull/13221) if you don't mind.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc removed a comment on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
junlincc removed a comment on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-782550377
@ktmud I do mind but go ahead
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-782550377
@ktmud I do mind but go ahead
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-781666197
tested locally, LGTM! ✅ Thank you for the fix!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud edited a comment on pull request #13211: fix(explore): Reset values in TextControl only when datasource changes
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #13211:
URL: https://github.com/apache/superset/pull/13211#issuecomment-782418459
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](https://github.com/apache/superset/pull/13221) if you don't mind.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org