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): change save button text when users cannot override #11281

Merged
merged 1 commit into from Oct 29, 2020

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Oct 15, 2020

SUMMARY

Change the save button text to "Save as new chart" when users cannot override an existing chart (normally because they are not the owner of the chart).

Wanted to add this change because I tried to edit a chart from someone else's dashboard today, after saving it I didn't see the change in the dashboard and got confused.

The text will only change if you are editing from an existing chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Open an existing chart that you are not an owner of, try save the chart:

Before

Snip20201014_25

After

Snip20201014_24

TEST PLAN

  • Creating a new chart: the button text should be "Save"
  • Editing a chart of your own: the text should be "Save"
  • Editing other people's chart: the text should be "Save as new chart"

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

@codecov-io
Copy link

codecov-io commented Oct 15, 2020

Codecov Report

Merging #11281 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11281   +/-   ##
=======================================
  Coverage   66.60%   66.60%           
=======================================
  Files         862      862           
  Lines       41212    41215    +3     
  Branches     3716     3719    +3     
=======================================
+ Hits        27449    27451    +2     
- Misses      13662    13663    +1     
  Partials      101      101           
Flag Coverage Δ
#cypress 56.42% <66.66%> (-0.02%) ⬇️
#javascript 62.71% <100.00%> (+<0.01%) ⬆️
#python 61.98% <ø> (ø)

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

Impacted Files Coverage Δ
...rset-frontend/src/explore/components/SaveModal.jsx 95.58% <100.00%> (+0.20%) ⬆️
.../src/dashboard/components/gridComponents/Chart.jsx 83.67% <0.00%> (-1.03%) ⬇️

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 18658f4...0bfef2e. Read the comment docs.

@ktmud
Copy link
Member Author

ktmud commented Oct 19, 2020

@mistercrunch @junlincc @zuzana-vej @graceguo-supercat do you think this change makes sense?

@zuzana-vej
Copy link
Contributor

zuzana-vej commented Oct 19, 2020

I think the new wording makes sense.
But if user selects "Save (Overwrite)" wouldn't "Update existing chart" or "Save (overwrite)" make more sense than just current "save"?

@ktmud
Copy link
Member Author

ktmud commented Oct 19, 2020

I think for other cases, "Save" works fine enough since users either need to explicitly change the value of the radio buttons or are using a reasonable default (save and overwrite for existing charts that they have write access). This PR handles a special case where the default is changed from "Overwrite" to "Save as" without an explicit call-out, so it's worth changing the button text to grab more user attention.

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM

@ktmud ktmud merged commit e050340 into apache:master Oct 29, 2020
@ktmud ktmud deleted the save-as-new branch October 29, 2020 16:41
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.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 size/XS 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants