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

chore(chart): Save modal select placeholder value #12413

Merged
merged 3 commits into from
Jan 20, 2021

Conversation

nikolagigic
Copy link
Contributor

SUMMARY

Fix SaveModal dashboard select placeholder value and clear button.
#12366

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
104073894-b578d680-51c3-11eb-81c3-d2714db4bef6
After:
Screenshot 2021-01-11 at 19 02 48

TEST PLAN

  1. Open or create any chart
  2. Click on Save button to save the chart

Expected result: Under ADD TO DASHBOARD label, select dropdown should have a placeholder text with no clear button. If the value is selected, the value should be displayed instead of placeholder and clear button should appear.

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

@nikolagigic
Copy link
Contributor Author

@junlincc @rusackas

@nikolagigic
Copy link
Contributor Author

nikolagigic commented Jan 11, 2021

🏷️ viz:explore:ui

@@ -29,7 +29,7 @@ import { connect } from 'react-redux';

// Session storage key for recent dashboard
const SK_DASHBOARD_ID = 'save_chart_recent_dashboard';
const SELECT_PLACEHOLDER = t('**Select** a dashboard OR **create** a new one');
const SELECT_PLACEHOLDER = t('Select dashboard');
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep the original copy as I don't think we've added the functionality to create a dashboard in the dropdown yet.

image

https://projects.invisionapp.com/share/V5VH03AHBER#/screens/399817652

cc @mihir174

For context, #10355 added this copy and there is a discussion on why this is still not optimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ktmud but the original text is referring to dashboard creation through save modal?

Copy link
Member

@ktmud ktmud Jan 11, 2021

Choose a reason for hiding this comment

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

Yes and I think we decided to allow users to do that since that's what in the original modal. Have we removed that functionality?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to create a dashboard through this modal and I guess text should claim that creation is possible.

@junlincc junlincc added the explore:save Related to saving changes in Explore label Jan 11, 2021
@adam-stasiak
Copy link
Contributor

adam-stasiak commented Jan 11, 2021

I have a behavior where when I add new chart and try to save it is automatically selected first dashboard from list:

Nagranie.z.ekranu.2021-01-11.o.20.57.37.mov

I guess it should not be chosen by default to avoid mistakes. In previous releases it was not proposed.
image

@codecov-io
Copy link

codecov-io commented Jan 11, 2021

Codecov Report

Merging #12413 (cca9364) into master (ff7b789) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12413      +/-   ##
==========================================
- Coverage   66.88%   66.76%   -0.13%     
==========================================
  Files        1014     1015       +1     
  Lines       49513    49644     +131     
  Branches     5077     4972     -105     
==========================================
+ Hits        33118    33144      +26     
- Misses      16264    16377     +113     
+ Partials      131      123       -8     
Flag Coverage Δ
cypress 51.01% <0.00%> (+0.03%) ⬆️
javascript 60.74% <100.00%> (+0.03%) ⬆️
python 63.96% <ø> (-0.26%) ⬇️

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

Impacted Files Coverage Δ
...rset-frontend/src/explore/components/SaveModal.tsx 91.01% <100.00%> (+0.42%) ⬆️
superset/examples/world_bank.py 30.98% <0.00%> (-69.02%) ⬇️
superset/examples/birth_names.py 73.19% <0.00%> (-25.65%) ⬇️
superset/examples/helpers.py 85.36% <0.00%> (-9.76%) ⬇️
...frontend/src/dashboard/containers/FiltersBadge.tsx 92.59% <0.00%> (-7.41%) ⬇️
superset/views/datasource.py 89.39% <0.00%> (-5.61%) ⬇️
...d/src/views/CRUD/csstemplates/CssTemplateModal.tsx 67.61% <0.00%> (-4.83%) ⬇️
...frontend/src/explore/components/DataTablesPane.tsx 58.90% <0.00%> (-4.48%) ⬇️
...src/dashboard/components/FiltersBadge/selectors.ts 83.78% <0.00%> (-4.46%) ⬇️
...et-frontend/src/dashboard/actions/sliceEntities.js 70.96% <0.00%> (-4.04%) ⬇️
... and 69 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 ff7b789...cca9364. Read the comment docs.

@ktmud
Copy link
Member

ktmud commented Jan 11, 2021

I guess it should not be chosen by default to avoid mistakes. In previous releases it was not proposed.

When users come from "View chart in Explore" on the dashboard page, the origin dashboard should be selected (so they can click on "Save & Go to Dashboard").

When users edit an existing chart from the Chart List page, the dashboard should not be selected.

(I think this is the current behavior and it's correct.)

@mihir174
Copy link
Contributor

thanks for the context @ktmud

Here's a common UI pattern I've seen in many products for this scenario -

Slice 1 (5)

@nikolagigic
Copy link
Contributor Author

Dashboards should now be selected properly and I changed the placeholder text

Copy link
Member

@junlincc junlincc 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 PR! @villebro @zhaoyongjie ready for code review. @ktmud take another look?

Explore save modal deserves a much bigger redesign. I can easily name a handful of UX issues.😞 Nothing critical, we can wait.

ezgif-7-3ca75cef6b86

Copy link
Member

@rusackas rusackas 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 to everyone for the input! I appreciate all the consideration (and empathy) that goes into these details!

@rusackas rusackas merged commit a422c76 into apache:master Jan 20, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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 explore:save Related to saving changes in Explore size/S 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants