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): Implement viz switcher redesign #20248

Merged
merged 15 commits into from
Jun 14, 2022

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Jun 2, 2022

SUMMARY

This PR implements the redesign of viz switcher. Now user can quickly change viz type to 1 of 6 featured charts without opening the viz gallery modal.
Active chart is represented by an icon and chart name. If chart's name is truncated, then tooltip appears on hover.
The rest of the featured charts are represented only by a clickable icon. Chart's name appears in a tooltip on hover. Clicking the icon switches the viz type.
User can open the viz gallery modal by clicking "View all charts". When user selects a chart that is not "featured", it's represented by a magnifying glass icon and chart's name. It's placed first on the quick viz switcher list.
When a non-featured chart is currently rendered, it's displayed at the start of the list.
Currently rendered chart has a tooltip "Currently rendered: "

Featured charts are:

  • Echarts Line Chart
  • Table
  • Big Number
  • Pie Chart
  • Echarts Bar Chart
  • Echarts Area Chart

Side note - this feature will work even better when Yongjie's standardized form data PR is merged 🙂

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-06-07.at.13.19.46.mov

TESTING INSTRUCTIONS

  1. Open explore with 1 of the featured charts
  2. Verify that there are 6 icons in viz switcher, the active viz is highlighted and its name is visible
  3. Click on any other icon, verify that the control panel now displays controls of currently active chart. Click Update Chart and verify that everything works as expected
  4. Click "View all charts", change to a non-featured chart
  5. Verify that now there are 7 items in viz switcher list and active non-featured chart is 1st on the list. Repeat step 3.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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

CC @kasiazjc

@kgabryje
Copy link
Member Author

kgabryje commented Jun 2, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2022

@kgabryje Ephemeral environment spinning up at http://52.41.74.164:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #20248 (f2ce49e) into master (2a45be3) will decrease coverage by 0.02%.
The diff coverage is 87.03%.

@@            Coverage Diff             @@
##           master   #20248      +/-   ##
==========================================
- Coverage   66.65%   66.62%   -0.03%     
==========================================
  Files        1729     1734       +5     
  Lines       64910    64997      +87     
  Branches     6842     6871      +29     
==========================================
+ Hits        43267    43306      +39     
- Misses      19894    19933      +39     
- Partials     1749     1758       +9     
Flag Coverage Δ
javascript 51.56% <87.03%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/components/Icons/index.tsx 100.00% <ø> (ø)
.../src/explore/components/ControlPanelsContainer.tsx 79.80% <ø> (ø)
...et-frontend/src/explore/controlPanels/sections.tsx 100.00% <ø> (ø)
...plore/components/controls/VizTypeControl/index.tsx 73.07% <66.66%> (-6.93%) ⬇️
...onents/controls/VizTypeControl/FastVizSwitcher.tsx 89.58% <89.58%> (ø)
...d/src/SqlLab/components/SaveDatasetModal/index.tsx 34.66% <0.00%> (-36.77%) ⬇️
...erset-ui-chart-controls/src/utils/columnChoices.ts 80.00% <0.00%> (-20.00%) ⬇️
...n-chart-handlebars/src/plugin/controls/orderBy.tsx 40.00% <0.00%> (-10.00%) ⬇️
...chart-controls/src/shared-controls/dndControls.tsx 26.92% <0.00%> (-8.98%) ⬇️
...nd/plugins/plugin-chart-table/src/controlPanel.tsx 43.47% <0.00%> (-7.27%) ⬇️
... and 23 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 2a45be3...f2ce49e. Read the comment docs.

@kgabryje
Copy link
Member Author

kgabryje commented Jun 3, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2022

@kgabryje Ephemeral environment spinning up at http://34.209.245.19:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@kasiazjc
Copy link
Contributor

kasiazjc commented Jun 3, 2022

Looks good to me!

One thing about the charts: we chose time-series (line, bar) as they will be becoming the default after the generic x-axis will be introduced for everyone. They have also similar statistics in terms of how much they are used (time series vs not).

Question: do we want to leave time-series charts, go back to simple line/bar OR add a logic which for example shows time-series ones if you have feature flag enabled and for others it shows basic line/bar? All of those are in Tier 1, so I think we need a product decision here. Thoughts @rusackas @lauderbaugh?

@jinghua-qa
Copy link
Member

I like the new feature, the only thing is when i open a non-feature chart, then select the viz switcher, my old chart viz will be gone and i need to go to "View all charts" to get my old chart viz back but then i have redo everything for my old chart. Can we have a better flow for undo selecting the viz switcher?

viz.switcher.mov

@zhaoyongjie
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

@zhaoyongjie Ephemeral environment spinning up at http://35.166.119.29:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@kgabryje kgabryje force-pushed the feat/viz-switcher branch from d3c0f23 to 4ea9ace Compare June 7, 2022 10:51
@kgabryje kgabryje force-pushed the feat/viz-switcher branch from 4ea9ace to c8bb67b Compare June 7, 2022 11:17
@kgabryje
Copy link
Member Author

kgabryje commented Jun 7, 2022

I like the new feature, the only thing is when i open a non-feature chart, then select the viz switcher, my old chart viz will be gone and i need to go to "View all charts" to get my old chart viz back but then i have redo everything for my old chart. Can we have a better flow for undo selecting the viz switcher?

Thanks for testing Jinghua! Now the currently rendered chart's icon stays visible at the start of the list when you switch between charts.
As for preventing the loss of control values when user switches chart, that's a part of Standardized Form Data project, implemented by @zhaoyongjie. Currently only a few charts support the feature of not losing data when viz type changes, but in the following weeks we'll implement it in most charts

@kgabryje
Copy link
Member Author

kgabryje commented Jun 7, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

@kgabryje Ephemeral environment spinning up at http://35.89.77.28:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

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.

Looks cool! I love this feature.

import { Tooltip } from 'src/components/Tooltip';
import Icons from 'src/components/Icons';
import { useSelector } from 'react-redux';
import { ExplorePageState } from '../../../reducers/getInitialState';
Copy link
Member

Choose a reason for hiding this comment

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

nit:

import { ExplorePageState } from 'src/explore/reducers/getInitialState';

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! I wish auto-import would use those absolute paths by default...

@lauderbaugh
Copy link
Contributor

Looks good to me!

One thing about the charts: we chose time-series (line, bar) as they will be becoming the default after the generic x-axis will be introduced for everyone. They have also similar statistics in terms of how much they are used (time series vs not).

Question: do we want to leave time-series charts, go back to simple line/bar OR add a logic which for example shows time-series ones if you have feature flag enabled and for others it shows basic line/bar? All of those are in Tier 1, so I think we need a product decision here. Thoughts @rusackas @lauderbaugh?

@kgabryje kgabryje merged commit 86f146e into apache:master Jun 14, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 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 need:qa-review Requires QA review preset-io size/XL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants