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

feat(explore): collapse time section if no ts columns #14493

Merged
merged 3 commits into from
May 7, 2021

Conversation

villebro
Copy link
Member

@villebro villebro commented May 5, 2021

SUMMARY

This is a follow-up of #12093 , which aims to automatically hide/expand the time section based on presence of temporal columns in the dataset.

Previously the ControlPanelsContainer component was using the defaultActiveKey prop in Collapse to control which panels were active. To be able to make the panels collapse automatically when we change datasources to one with/without temporal columns, we have to switch to activeKey and manage the state within the component. To do this some code had to be moved out of the render method, but the majority of the logic is untouched.

Previous flow:

  • Time section is always expanded by default, but possible to collapse/expand manually
  • Changing to a datasource without any temporal columns did not collapse the time section.

New flow:

  • By default the time section opens up in a collapsed state if there is no temporal column in the dataset
  • Time section can still be manually expanded/collapsed manually like before
  • By changing to a dataset with (without) at least one temporal column will cause the time section to expand (collapse) the time section.
  • When changing viz type, the time section will be automatically expanded/collapsed based on the logic described above

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Previously the time section was always expanded, and changing the dataset to/from one with/without any temporal columns didn't change anything:
https://user-images.githubusercontent.com/33317356/117162820-ca5d2380-adcb-11eb-923d-d1281668f5a0.mp4

AFTER

Now the time section is automatically expanded/collapsed when changing to a dataset with/without a temporal column.
https://user-images.githubusercontent.com/33317356/117162867-d2b55e80-adcb-11eb-9dae-3f26fcaf9e45.mp4

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@villebro villebro changed the title feat(explore): collapse time section if no ts columns [WIP] feat(explore): collapse time section if no ts columns May 5, 2021
@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #14493 (20cbcd8) into master (19b408b) will decrease coverage by 0.00%.
The diff coverage is 78.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14493      +/-   ##
==========================================
- Coverage   76.90%   76.90%   -0.01%     
==========================================
  Files         958      958              
  Lines       48241    48268      +27     
  Branches     5636     5645       +9     
==========================================
+ Hits        37102    37120      +18     
- Misses      10938    10947       +9     
  Partials      201      201              
Flag Coverage Δ
javascript 72.00% <78.43%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
.../src/explore/components/ControlPanelsContainer.tsx 81.42% <78.43%> (-3.53%) ⬇️

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 19b408b...20cbcd8. Read the comment docs.

@@ -111,7 +111,7 @@ describe('VizType control', () => {
// should load mathjs for line chart
cy.get('script[src*="mathjs"]').should('have.length', 1);
cy.get('script').then(nodes => {
expect(nodes.length).to.eq(numScripts);
expect(nodes.length).to.greaterThan(numScripts);
Copy link
Member Author

@villebro villebro May 6, 2021

Choose a reason for hiding this comment

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

I wasn't able to reproduce this locally (locally I was only getting 11 script nodes, but cypress is finding nodes.length === 13, numScripts === 12), but I noticed that this used to be greaterThan before this PR: #12218 , so I assume this can be flipped back.

@villebro villebro changed the title [WIP] feat(explore): collapse time section if no ts columns feat(explore): collapse time section if no ts columns May 6, 2021
@zhaoyongjie zhaoyongjie self-requested a review May 7, 2021 08:19
Copy link
Member

@zhaoyongjie zhaoyongjie 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, Ville.

@villebro villebro merged commit 680c96e into apache:master May 7, 2021
@villebro villebro deleted the villebro/time-section branch May 7, 2021 08:56
@rusackas rusackas added bash! and removed bash! labels May 12, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* feat(explore): collapse time section if no ts columns

* fix viz change bug

* fix test
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* feat(explore): collapse time section if no ts columns

* fix viz change bug

* fix test
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* feat(explore): collapse time section if no ts columns

* fix viz change bug

* fix test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.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 preset-io size/L 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants