-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@kgabryje Ephemeral environment spinning up at http://54.189.133.181:8080. Credentials are |
Three observations:
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:
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.mp4After: ex_after2.mp4 |
Good point, will change!
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.
Oooooops...
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 :) |
@villebro Fixes implemented.
|
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:
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 😉 |
@michael-s-molina Thanks for comments! Sneak preview for the replacement of overlay with "run query" button: 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. |
/testenv up |
@villebro Ephemeral environment spinning up at http://35.160.60.86:8080. Credentials are |
superset-frontend/src/explore/components/ControlPanelsContainer.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/ControlPanelsContainer.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/ControlPanelsContainer.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/index.jsx
Show resolved
Hide resolved
superset-frontend/src/explore/components/ControlPanelsContainer.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/RunQueryButton/RunQueryButton.test.tsx
Outdated
Show resolved
Hide resolved
51b638b
to
74828b9
Compare
74828b9
to
c528621
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And thanks for feedback and discussion @michael-s-molina 💞 |
/testenv up |
There was a problem hiding this 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
@geido Ephemeral environment spinning up at http://18.237.253.23:8080. Credentials are |
@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. |
Makes sense. Thanks! |
Ephemeral environment shutdown and build artifacts deleted. |
* 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
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:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2022-04-06.at.13.06.55.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
@kasiazjc