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): Move chart actions into dropdown #19446

Merged
merged 12 commits into from
Mar 31, 2022

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Mar 30, 2022

SUMMARY

This PR is a stage 1 of chart header redesign.
It focuses on moving the action button into a dropdown menu to declutter the header.

List of changes:

  • move "download json/csv/image" buttons to "Download" submenu of actions dropdown
  • move "Set up a report" to dropdown if there's no report
  • move "Edit report", "Enable/disable report" and "Delete report" to "Manage the report" submenu of actions dropdown
  • move embed code button and share by email button to dropdown
  • move copy permalink button to the left side of the header, next to "favourite" button
  • refactor embed code popover to be a modal, change styling of modal's content

Final design available in discussion: #19099

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-03-30.at.21.13.39.mov

TESTING INSTRUCTIONS

Verify that all action in the dropdown menu work exactly the same as they did before moving them to dropdown

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

@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #19446 (28d0c0f) into master (6b136c2) will increase coverage by 0.00%.
The diff coverage is 60.26%.

@@           Coverage Diff           @@
##           master   #19446   +/-   ##
=======================================
  Coverage   66.57%   66.58%           
=======================================
  Files        1675     1676    +1     
  Lines       64092    64172   +80     
  Branches     6519     6525    +6     
=======================================
+ Hits        42672    42731   +59     
- Misses      19729    19742   +13     
- Partials     1691     1699    +8     
Flag Coverage Δ
javascript 51.31% <60.26%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
...set-frontend/src/components/ModalTrigger/index.jsx 100.00% <ø> (ø)
...frontend/src/dashboard/components/Header/index.jsx 60.92% <ø> (ø)
...rc/explore/components/ExploreChartHeader/index.jsx 50.00% <ø> (+1.31%) ⬆️
.../components/ExploreAdditionalActionsMenu/index.jsx 57.14% <55.00%> (-38.32%) ⬇️
...nts/ExploreAdditionalActionsMenu/ExploreReport.tsx 57.89% <57.89%> (ø)
...ontend/src/explore/components/EmbedCodeContent.jsx 76.66% <76.66%> (ø)
...rset-frontend/src/components/ReportModal/index.tsx 78.46% <100.00%> (-3.63%) ⬇️
...set-frontend/src/components/ReportModal/styles.tsx 87.50% <0.00%> (-12.50%) ⬇️
...plugins/legacy-plugin-chart-heatmap/src/Heatmap.js 0.00% <0.00%> (ø)
... and 8 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 6b136c2...28d0c0f. Read the comment docs.

@villebro
Copy link
Member

It feels strange that the "Share chart by email" and "Embed code" are in the dropdown, while the permalink button is in the header, as I assume people will consider them similar use cases. I can see myself clicking on the action button and wondering where the permalink item is. Would one of these make sense?

  1. Making a "Share chart" submenu where we would have "Copy permalink", "Embed code" and "Share chart by email". We could potentially leave the permalink button where it is, but this way all the sharing/permalink-based functionality could at least be found in the same place
  2. Add "Copy permalink" to the Action menu. This would cause some duplication, but then it would also be located close to the other similar functionality
  3. Move the permalink button into the Action menu all together. This would have the benefit of decluttering the header even further (I'm not sure how common it is to copy permalinks)
  4. Turn the permalink button a dropdown menu with the different sharing options (copy permalink, share via email and embed code)

@kgabryje
Copy link
Member Author

It feels strange that the "Share chart by email" and "Embed code" are in the dropdown, while the permalink button is in the header, as I assume people will consider them similar use cases. I can see myself clicking on the action button and wondering where the permalink item is. Would one of these make sense?

  1. Making a "Share chart" submenu where we would have "Copy permalink", "Embed code" and "Share chart by email". We could potentially leave the permalink button where it is, but this way all the sharing/permalink-based functionality could at least be found in the same place
  2. Add "Copy permalink" to the Action menu. This would cause some duplication, but then it would also be located close to the other similar functionality
  3. Move the permalink button into the Action menu all together. This would have the benefit of decluttering the header even further (I'm not sure how common it is to copy permalinks)
  4. Turn the permalink button a dropdown menu with the different sharing options (copy permalink, share via email and embed code)

Thanks for comments. Personally I like the 1st option the most, but I guess @kasiazjc would probably want to chime in 🙂

@kasiazjc
Copy link
Contributor

It feels strange that the "Share chart by email" and "Embed code" are in the dropdown, while the permalink button is in the header, as I assume people will consider them similar use cases. I can see myself clicking on the action button and wondering where the permalink item is. Would one of these make sense?

  1. Making a "Share chart" submenu where we would have "Copy permalink", "Embed code" and "Share chart by email". We could potentially leave the permalink button where it is, but this way all the sharing/permalink-based functionality could at least be found in the same place
  2. Add "Copy permalink" to the Action menu. This would cause some duplication, but then it would also be located close to the other similar functionality
  3. Move the permalink button into the Action menu all together. This would have the benefit of decluttering the header even further (I'm not sure how common it is to copy permalinks)
  4. Turn the permalink button a dropdown menu with the different sharing options (copy permalink, share via email and embed code)

Thanks for comments. Personally I like the 1st option the most, but I guess @kasiazjc would probably want to chime in 🙂

Good point @villebro! I agree, 1st option would work best. In that case we can remove the icon from the header So we don't duplicate the actions.

@kgabryje
Copy link
Member Author

@kasiazjc @villebro Thanks for feedback, here's how it looks after moving copy permalink to menu 🙂
image

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM with a few minor comments

import * as chartAction from 'src/components/Chart/chartAction';
import * as downloadAsImage from 'src/utils/downloadAsImage';
import fetchMock from 'fetch-mock';
import * as exploreUtils from 'src/explore/exploreUtils';
Copy link
Member

Choose a reason for hiding this comment

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

nit: importing * always makes me cringe, but in a test I guess it's acceptable 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like that either, but I think that syntax is necessary to use sinon.spy

<Menu.Item key={MENU_KEYS.EMBED_CODE}>
<ModalTrigger
triggerNode={
<span data-test="embed-code-button">{t('Embed code')}</span>
Copy link
Member

Choose a reason for hiding this comment

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

minor comment: I'd kinda prefer to see "Embed code" below "Share chart via email"

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@kgabryje kgabryje merged commit 1a1322d into apache:master Mar 31, 2022
@villebro villebro added lts-v1 and removed lts-v1 labels Apr 1, 2022
@villebro villebro mentioned this pull request Apr 6, 2022
8 tasks
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* feat(explore): Move chart actions to a dropdown menu

* Fix tests and add some new ones

* Add background color to embed code button

* Fix cypress tests

* Move copy permalink to actions menu

* Remove unused function

* Move share by email above embed code

* Fix test

* Fix test

* Fix test

* Fix test

* Fix test
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label 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/XXL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants