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): Redesign of Run/Save buttons #19558

Merged
merged 11 commits into from
Apr 13, 2022

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Apr 6, 2022

SUMMARY

This PR moves the "Save" button to chart header and "Run" button (renamed to "Create/Update chart") to the bottom of control panel.

Full list of changes:

  • "Save" button moved to the right side if chart header
  • Renamed "Run" button to "Create chart" if it's a new chart (no query has been made yet) or "Update chart" if queries were run before on that chart
  • Moved "Create/Update chart" button to the bottom of control panel - it's sticky, meaning it always stays in place when user scrolls the control panel
  • "Data/Customize" tab bar is sticky - only the content of tab gets scrolled, the tab bar stays in place
  • Error icon moved from next to "Run/Save" buttons to "Data" tab header
  • Change empty state message when creating chart and all controls have been added - "Your chart is ready to go!"

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-04-06.at.13.06.55.mov

TESTING INSTRUCTIONS

  1. Create new chart
  2. Verify that the button at the bottom of control panel says "Create chart" and is disabled
  3. Add necessary controls
  4. "Create chart" button should not be disabled now + the message in empty state changed to a prompt to click the button
  5. Click "Create chart" button
  6. Verify that query was run and the name of the button changed to "Update chart"
  7. Verify that "Save" button in the chart header works as before

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

@kasiazjc

@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #19558 (d086283) into master (a6bf041) will decrease coverage by 0.00%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##           master   #19558      +/-   ##
==========================================
- Coverage   66.45%   66.45%   -0.01%     
==========================================
  Files        1681     1681              
  Lines       64389    64410      +21     
  Branches     6592     6597       +5     
==========================================
+ Hits        42792    42803      +11     
- Misses      19914    19926      +12     
+ Partials     1683     1681       -2     
Flag Coverage Δ
javascript 51.15% <77.27%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/components/Chart/Chart.jsx 50.00% <0.00%> (-2.46%) ⬇️
.../components/ExploreAdditionalActionsMenu/index.jsx 57.14% <ø> (ø)
.../src/explore/components/ControlPanelsContainer.tsx 79.80% <71.42%> (+0.21%) ⬆️
...rc/explore/components/ExploreChartHeader/index.jsx 43.10% <100.00%> (+0.99%) ⬆️
.../explore/components/ExploreViewContainer/index.jsx 54.89% <100.00%> (+0.49%) ⬆️
...nd/src/explore/components/RunQueryButton/index.tsx 100.00% <100.00%> (ø)
...rontend/src/components/ListView/Filters/Search.tsx 70.58% <0.00%> (-7.99%) ⬇️
superset-frontend/src/components/Icons/Icon.tsx 95.23% <0.00%> (-4.77%) ⬇️
...rset-frontend/src/components/ListView/ListView.tsx 94.11% <0.00%> (-4.64%) ⬇️
...erset-frontend/src/components/EmptyState/index.tsx 71.42% <0.00%> (-2.94%) ⬇️
... and 10 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 a6bf041...d086283. Read the comment docs.

@kgabryje
Copy link
Member Author

kgabryje commented Apr 6, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

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

@villebro
Copy link
Member

villebro commented Apr 8, 2022

Three observations:

  1. Currently the Run button only becomes "enabled" (well, it's not really ever disabled, the background just becomes white) when the query result is stale from controls that affect the queries (see before video). In the new version, the "Update chart" button is "enabled" all the time (see after video).
  2. The "Run Query" in the chart container should probably be changed to "Update chart" to be in line with the other button.
  3. When there are errors, currently the Save button is disabled. In the new version, the Save button appears to be enabled all the time.

A more general observation: I believe one of the reasons the button is currently called "Run" is to reflect that it executes a query (changing controls that only affect the visuals of the chart don't require rerunning the chart, as they're applied on the fly). Renaming it to "Create chart" and "Update chart" may cause confusing due to the following:

  • Users will think they have to press "Update chart" when they make changes that don't affect the query
  • When the button is called "Create chart", there's risk that users will think it will also save the chart.

However, my comments should be taken with a grain of salt, as I'm probably too used to the old behavior.

Other than that I love the new design!

Before:

ex_before.mp4

After:

ex_after2.mp4

@kgabryje
Copy link
Member Author

kgabryje commented Apr 8, 2022

  1. Currently the Run button only becomes "enabled" (well, it's not really ever disabled, the background just becomes white) when the query result is stale from controls that affect the queries (see before video). In the new version, the "Update chart" button is "enabled" all the time (see after video).

Good point, will change!

  1. The "Run Query" in the chart container should probably be changed to "Update chart" to be in line with the other button.

Today I'm going to open a PR that totally removes the overlay with "Run query". It will be replaced by an alert banner that tells the user that something has changed in the controls. Thanks to that, the user will still be able to see the chart while making changes in the controls.

  1. When there are errors, currently the Save button is disabled. In the new version, the Save button appears to be enabled all the time.

Oooooops...

A more general observation: I believe one of the reasons the button is currently called "Run" is to reflect that it executes a query (changing controls that only affect the visuals of the chart don't require rerunning the chart, as they're applied on the fly). Renaming it to "Create chart" and "Update chart" may cause confusing due to the following:

  • Users will think they have to press "Update chart" when they make changes that don't affect the query
  • When the button is called "Create chart", there's risk that users will think it will also save the chart.

I'll let @kasiazjc address that one 🙂

@kasiazjc
Copy link
Contributor

kasiazjc commented Apr 8, 2022

  1. Currently the Run button only becomes "enabled" (well, it's not really ever disabled, the background just becomes white) when the query result is stale from controls that affect the queries (see before video). In the new version, the "Update chart" button is "enabled" all the time (see after video).

Good point, will change!

  1. The "Run Query" in the chart container should probably be changed to "Update chart" to be in line with the other button.

Today I'm going to open a PR that totally removes the overlay with "Run query". It will be replaced by an alert banner that tells the user that something has changed in the controls. Thanks to that, the user will still be able to see the chart while making changes in the controls.

  1. When there are errors, currently the Save button is disabled. In the new version, the Save button appears to be enabled all the time.

Oooooops...

A more general observation: I believe one of the reasons the button is currently called "Run" is to reflect that it executes a query (changing controls that only affect the visuals of the chart don't require rerunning the chart, as they're applied on the fly). Renaming it to "Create chart" and "Update chart" may cause confusing due to the following:

  • Users will think they have to press "Update chart" when they make changes that don't affect the query
  • When the button is called "Create chart", there's risk that users will think it will also save the chart.

I'll let @kasiazjc address that one 🙂

@villebro thanks for ten Feedback 🙏🏻

For the copy we got feedback via Aha/GitHub, that "run query" is confusing as some people do not associate creating chart with running the query.

Hopefully "save" button in the header should solve the second problem.

I was also thinking about naming the button "preview chart", but then it completely removes the indication that the query was ran, as preview seems less... invasive if that makes sense.

Let's stick with this copy for now and we'll iterate if needed :)

@kgabryje
Copy link
Member Author

kgabryje commented Apr 8, 2022

@villebro Fixes implemented.

  1. Update button is now secondary instead of primary if chart is not stale

image

  1. Save button is disabled when there are errors + tooltip

image

@michael-s-molina
Copy link
Member

michael-s-molina commented Apr 8, 2022

Users will think they have to press "Update chart" when they make changes that don't affect the query

I agree with @villebro on this one

@kasiazjc @kgabryje I really like the new design, things are getting less cluttered, and the Explore screen seems simpler while keeping the same features. I do have a different preference for the organization for the create/update buttons though. I personally don't like things floating on top of other things, especially on top of the configuration panel where we have many controls. It gives me a crowded feeling.

One idea would be to move the create button directly below the empty message, it seems very intuitive because I'm reading that my chart is ready to go and there would be a button right there to create it.

About the update, I liked the previous version that showed an overlay on top of the chart with a button to update. I like this approach because:

  • It shows the user (with the overlay + button) that they did a configuration that needs an explicit update request
  • The button to update is right there on the overlay. We could even add a message indicating that the user needs to update to see the changes reflected on the chart
  • When a user does a configuration on the left panel that doesn't require an explicit update request, the chart simply updates and there's no button on the screen that could lead to confusion.

For the case where the user wants to re-execute the query without having made any modification to the configurations, to bust the cache, for example, we could have a "Refresh data" option in the secondary actions popover.

These are just ideas, I don't consider them as blockers for the PR in case you choose a different direction 😉

@kgabryje
Copy link
Member Author

kgabryje commented Apr 8, 2022

@michael-s-molina Thanks for comments! Sneak preview for the replacement of overlay with "run query" button:

image

When a control that's not a "render trigger" changes, user sees an alert banner. That lets them know that they should click update chart button, but at the same time they can still see their chart.

@michael-s-molina
Copy link
Member

When a control that's not a "render trigger" changes, user sees an alert banner. That lets them know that they should click update chart button, but at the same time they can still see their chart.

That's better than the overlay! I like that the user can see the chart. I would just add the update button right there instead of floating on the left panel.

@villebro
Copy link
Member

villebro commented Apr 8, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2022

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

Copy link
Member

@michael-s-molina michael-s-molina 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 for the awesome changes @kgabryje @kasiazjc!

@kasiazjc
Copy link
Contributor

LGTM. Thanks for the awesome changes @kgabryje @kasiazjc!

And thanks for feedback and discussion @michael-s-molina 💞

@geido
Copy link
Member

geido commented Apr 11, 2022

/testenv up

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

Code looks good to me. I'd like to have a manual testing round when the testenv spins up

@github-actions
Copy link
Contributor

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

@geido
Copy link
Member

geido commented Apr 11, 2022

@kgabryje not sure if this is intended but when you switch viz types, the button stays at "UPDATE CHART". This happens when some metrics are kept and also when all metrics are dropped (basically starting from a blank state). I think it should say "CREATE CHART" to be consistent, especially in the latter case where no metrics are kept.

@kgabryje
Copy link
Member Author

@kgabryje not sure if this is intended but when you switch viz types, the button stays at "UPDATE CHART". This happens when some metrics are kept and also when all metrics are dropped (basically starting from a blank state). I think it should say "CREATE CHART" to be consistent, especially in the latter case where no metrics are kept.

That's a really good question. It was intentional, yes, because from my point of view if a chart exists (i.e. it's been rendered), then changing the viz type is updating, not creating from scratch, even if controls were reset. After all, that chart is going to have the same reference at the list of charts and is going to take the old chart's place in any dashboard where it was added. So I think in that case we're updating a chart, not creating something new.
What do you think?

@geido
Copy link
Member

geido commented Apr 13, 2022

@kgabryje not sure if this is intended but when you switch viz types, the button stays at "UPDATE CHART". This happens when some metrics are kept and also when all metrics are dropped (basically starting from a blank state). I think it should say "CREATE CHART" to be consistent, especially in the latter case where no metrics are kept.

That's a really good question. It was intentional, yes, because from my point of view if a chart exists (i.e. it's been rendered), then changing the viz type is updating, not creating from scratch, even if controls were reset. After all, that chart is going to have the same reference at the list of charts and is going to take the old chart's place in any dashboard where it was added. So I think in that case we're updating a chart, not creating something new.
What do you think?

Makes sense. Thanks!

@kgabryje kgabryje merged commit c8304a2 into apache:master Apr 13, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* feat(explore): Move save button to header, run button to bottom of control panel

* Make the tabs sticky

* Add error icon to Data tab

* Show message when creating chart and all controls are filled correctly

* Add tests and storybook

* Fix tests

* Disable save button when control have errors

* Fix types

* Apply code review comments

* Replace styled with css

* Remove unused import
@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 size/XL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants