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 2020/04/09 00:26:03 UTC
[GitHub] [incubator-superset] ktmud opened a new pull request #9495: Add
explore control tabOverride at the section level
ktmud opened a new pull request #9495: Add explore control tabOverride at the section level
URL: https://github.com/apache/incubator-superset/pull/9495
### CATEGORY
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
https://github.com/apache/incubator-superset/pull/9440 unintentionally moved the "Options" section for Big Number chart to the "Customize" tab.
This is because [ControlPanelsContainer](https://github.com/apache/incubator-superset/blob/06c4795fba48ed64e54e74eb8eb2f5a202c01fc8/superset-frontend/src/explore/components/ControlPanelsContainer.jsx#L203) used to check whether a section needs to display in the Data tab by checking whether any of the control in that section needs to run a new query when updated (`renderTrigger: false`) or has `tabOverride = data`. But it uses the config in the shared controls file, and assumes the control configs passed along are control name strings, and could be used to find the shared control config. By refactoring them into custom config, it fails to look up the shared control, hence the `renderTrigger` and `tabOverride` info.
Ideally this `tabOverride` option should probably be at the section level.
A bigger refactor is need to let `ControlPanelsContainer` always read from parsed control configs (instead of the control keys). This PR is just a quick fix.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
#### Before
The "Options" section is displayed in the "Customize" tab.
#### After
The "Options" section is displayed in the "Data" tab like before.
### TEST PLAN
- Check any big number chart with trendlines.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [x] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@rusackas @michellethomas
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on issue #9495: Add explore
control tabOverride at the section level
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9495: Add explore control tabOverride at the section level
URL: https://github.com/apache/incubator-superset/pull/9495#issuecomment-611349876
Thanks for catching this AND for fixing it! I'll keep an eye out for this in the future.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kristw merged pull request #9495: Add explore
control tabOverride at the section level
Posted by GitBox <gi...@apache.org>.
kristw merged pull request #9495: Add explore control tabOverride at the section level
URL: https://github.com/apache/incubator-superset/pull/9495
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] michellethomas commented on a change in pull
request #9495: Add explore control tabOverride at the section level
Posted by GitBox <gi...@apache.org>.
michellethomas commented on a change in pull request #9495: Add explore control tabOverride at the section level
URL: https://github.com/apache/incubator-superset/pull/9495#discussion_r405898508
##########
File path: superset-frontend/src/explore/components/ControlPanelsContainer.jsx
##########
@@ -198,7 +198,10 @@ class ControlPanelsContainer extends React.Component {
const querySectionsToRender = [];
const displaySectionsToRender = [];
allSectionsToRender.forEach(section => {
+ // if at least one control in the secion is not `renderTrigger`
+ // or asks to be displayed at the Data tab
if (
+ section.tabOverride === 'data' ||
Review comment:
The lines 208-210 below should be changed too right (could be in another PR so we can fix this soon)? To account for control potentially being a string or an object.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] michellethomas commented on a change in pull
request #9495: Add explore control tabOverride at the section level
Posted by GitBox <gi...@apache.org>.
michellethomas commented on a change in pull request #9495: Add explore control tabOverride at the section level
URL: https://github.com/apache/incubator-superset/pull/9495#discussion_r405898508
##########
File path: superset-frontend/src/explore/components/ControlPanelsContainer.jsx
##########
@@ -198,7 +198,10 @@ class ControlPanelsContainer extends React.Component {
const querySectionsToRender = [];
const displaySectionsToRender = [];
allSectionsToRender.forEach(section => {
+ // if at least one control in the secion is not `renderTrigger`
+ // or asks to be displayed at the Data tab
if (
+ section.tabOverride === 'data' ||
Review comment:
The lines 208-210 below should be changed to account for control potentially being a string or an object, right? I wouldn't block this PR on that change since we want to get the fix in, just making a note because it might cause this issue again in the future.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request
#9495: Add explore control tabOverride at the section level
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9495: Add explore control tabOverride at the section level
URL: https://github.com/apache/incubator-superset/pull/9495#discussion_r405898910
##########
File path: superset-frontend/src/explore/components/ControlPanelsContainer.jsx
##########
@@ -198,7 +198,10 @@ class ControlPanelsContainer extends React.Component {
const querySectionsToRender = [];
const displaySectionsToRender = [];
allSectionsToRender.forEach(section => {
+ // if at least one control in the secion is not `renderTrigger`
+ // or asks to be displayed at the Data tab
if (
+ section.tabOverride === 'data' ||
Review comment:
Yes, I think we should preprocess the configs at some point and avoid accounting for two types. I'll see what I can do later.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] michellethomas commented on issue #9495: Add
explore control tabOverride at the section level
Posted by GitBox <gi...@apache.org>.
michellethomas commented on issue #9495: Add explore control tabOverride at the section level
URL: https://github.com/apache/incubator-superset/pull/9495#issuecomment-611270015
Thanks for fixing this!
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org