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

refactor: Simplifying tabOverride logic in control sections #11708

Closed
wants to merge 5 commits into from

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Nov 16, 2020

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Various control sections have been showing up under unexpected tabs. This PR makes it simpler/easier to shove control sections under the correct tab, by allowing "tabOverride" to be either "data" or "customize" and simplifying (essentially inverting) the logic that takes place when neither is specified. This also removes support for putting the "tabOverride" in a singular control, and just keeps it to the control section, for simplicity.

I have half a mind to just make it "tab" instead of "tabOverride" and bake it into every control section, as a standard practice... but I'll pocket that one, as a control layout refactor will likely be coming soon (reconsidering the current rows/columns approach). But that's another story, and probably another PR...

Also, there will be a PR over on superset-ui that utilizes this new "customize" value in numerous places. We can merge that one before this one, I think, and I'll bump all the chart versions as part of this PR when it exits draft mode.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@rusackas rusackas requested a review from ktmud November 16, 2020 04:40
@rusackas rusackas changed the title refactor: Simplifying tabOverride logic in control sections refactor: WIP Simplifying tabOverride logic in control sections Nov 16, 2020
@codecov-io
Copy link

codecov-io commented Nov 16, 2020

Codecov Report

Merging #11708 (dd67d59) into master (44e80e0) will decrease coverage by 3.08%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11708      +/-   ##
==========================================
- Coverage   66.79%   63.71%   -3.09%     
==========================================
  Files         924      924              
  Lines       44837    44828       -9     
  Branches     4253     4247       -6     
==========================================
- Hits        29951    28562    -1389     
- Misses      14777    16089    +1312     
- Partials      109      177      +68     
Flag Coverage Δ
cypress ?
javascript 63.03% <75.00%> (+<0.01%) ⬆️
python 64.11% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...et-frontend/src/explore/controlPanels/sections.jsx 100.00% <ø> (ø)
superset-frontend/src/explore/controls.jsx 26.82% <ø> (ø)
.../src/explore/components/ControlPanelsContainer.jsx 75.36% <75.00%> (-17.50%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
... and 160 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44e80e0...dd67d59. Read the comment docs.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Nov 16, 2020
@ktmud
Copy link
Member

ktmud commented Nov 16, 2020

Related: #9495

@rusackas rusackas force-pushed the control-section-touchups branch 2 times, most recently from a05dcd8 to fbf8cd8 Compare November 17, 2020 07:36
@rusackas rusackas marked this pull request as ready for review November 17, 2020 07:51
@rusackas rusackas requested a review from kristw November 17, 2020 07:51
@rusackas
Copy link
Member Author

@ktmud / @kristw I opened this for review, as I think it's safe enough to merge on its own regardless of what I muddle around with over on the superset-ui side, but please check my sanity on this :D

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a customize option, I'd be more enthusiastic on getting rid of the "renderTrigger implies data tab" logic. To really keep things simple, we should just always default things to be on the customize tab and require explicit override if people want to display things in the Data tab (or other way around if it makes more sense).

But this change is probably safe since I didn't see any use of tabOverride at the control item level anymore.

superset-frontend/src/explore/controls.jsx Outdated Show resolved Hide resolved
@rusackas rusackas force-pushed the control-section-touchups branch from fbf8cd8 to 4a86bd0 Compare December 1, 2020 07:43
@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 1, 2020
@rusackas
Copy link
Member Author

rusackas commented Dec 1, 2020

Instead of adding a customize option, I'd be more enthusiastic on getting rid of the "renderTrigger implies data tab" logic. To really keep things simple, we should just always default things to be on the customize tab and require explicit override if people want to display things in the Data tab (or other way around if it makes more sense).

But this change is probably safe since I didn't see any use of tabOverride at the control item level anymore.

Yep... I had an itchy trigger finger to remove that behavior entirely, but didn't want to cause any weird regressions. I'll do that in another PR once we've made some more updates on the superset-ui side and merged them in.

@rusackas rusackas changed the title refactor: WIP Simplifying tabOverride logic in control sections refactor: Simplifying tabOverride logic in control sections Dec 1, 2020
@rusackas rusackas force-pushed the control-section-touchups branch from dd67d59 to fc16027 Compare December 8, 2020 22:20
@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jun 26, 2021
@amitmiran137
Copy link
Member

closing this due to inactivity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants