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

allow saving chart images #28546

Merged
merged 7 commits into from
Feb 23, 2023
Merged

allow saving chart images #28546

merged 7 commits into from
Feb 23, 2023

Conversation

alxnddr
Copy link
Member

@alxnddr alxnddr commented Feb 22, 2023

Closes #4701

Description

A last-minute change to allow users to save png images of charts

How to verify

  1. Create a line/area/bar/pie/gauge/... chart and ensure you can save it
  2. Add the charts on a dashboard and ensure you can save images from dashcards

Saving images does not work on visualizations that have canSavePng=false such as tables, etc.

Demo

QB:

Screen Shot 2023-02-22 at 3 17 42 PM

Dashcards:

Screen Shot 2023-02-22 at 3 18 22 PM

Checklist

  • Tests have been added/updated to cover changes in this PR

@alxnddr alxnddr self-assigned this Feb 22, 2023
@alxnddr alxnddr added this to the 0.46 milestone Feb 22, 2023
@kdoh kdoh mentioned this pull request Feb 22, 2023
5 tasks
Copy link
Member

@kdoh kdoh left a comment

Choose a reason for hiding this comment

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

Behavior works as expected when I was testing it locally. 👍

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Base: 67.37% // Head: 67.38% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (9cb5e8b) compared to base (42d973b).
Patch coverage: 20.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #28546      +/-   ##
==========================================
+ Coverage   67.37%   67.38%   +0.01%     
==========================================
  Files        3328     3329       +1     
  Lines       97018    97098      +80     
  Branches    12299    12308       +9     
==========================================
+ Hits        65363    65433      +70     
- Misses      26547    26556       +9     
- Partials     5108     5109       +1     
Flag Coverage Δ
back-end 85.54% <ø> (+0.03%) ⬆️
front-end 50.41% <20.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...etabase/actions/components/ActionViz/ActionViz.tsx 25.00% <ø> (ø)
...oard/components/DashCard/DashCardVisualization.tsx 4.00% <ø> (ø)
frontend/src/metabase/icon_paths.ts 96.29% <ø> (ø)
.../src/metabase/public/containers/PublicQuestion.jsx 0.00% <ø> (ø)
...e/query_builder/components/QueryDownloadWidget.jsx 20.00% <0.00%> (-0.32%) ⬇️
...se/query_builder/components/QueryVisualization.jsx 0.00% <ø> (ø)
...omponents/components/GlobalStyles/GlobalStyles.tsx 0.00% <ø> (ø)
...zations/components/Visualization/Visualization.jsx 39.37% <0.00%> (ø)
frontend/src/metabase/visualizations/index.js 67.79% <0.00%> (-2.38%) ⬇️
...izations/visualizations/LinkViz/LinkVizSettings.ts 33.33% <ø> (ø)
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@deploysentinel
Copy link

deploysentinel bot commented Feb 22, 2023

No failed tests 🎉

@alxnddr alxnddr requested review from a team and nemanjaglumac February 22, 2023 22:42
@iethree iethree self-requested a review February 22, 2023 23:47
Comment on lines +146 to +147
// FIXME: update PNG icon
png: "M28 10.105v18.728A3.166 3.166 0 0 1 24.834 32H6.166A3.163 3.163 0 0 1 3 28.844V3.156A3.163 3.163 0 0 1 6.16 0h13.553V10.105H28zm-.215-1.684h-6.4V.311l6.4 8.11zM17 13v2h2v-2h-2zm0 4v2h2v-2h-2zm4-4v2h2v-2h-2zM7 13v2h7v-2H7zm14 4v2h2v-2h-2zM7 17v2h7v-2H7zm10 4v2h2v-2h-2zm4 0v2h2v-2h-2zM7 21v2h7v-2H7z",
Copy link
Contributor

Choose a reason for hiding this comment

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

@kdoh were you working on an icon for this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes but it's not blocking to get this out.

@iethree
Copy link
Contributor

iethree commented Feb 23, 2023

saving from dashboards works great, but I'm getting 0 byte downloads for downloads directly from questions. the console output leads me to believe it thinks the height is 0px?

Screen Shot 2023-02-22 at 5 00 39 PM

Copy link
Contributor

@npfitz npfitz left a comment

Choose a reason for hiding this comment

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

I did notice in the network tab that html2canvas does generate a small 1px gif, maybe that's what Ryan is referring to. I don't think it's an issue though.

From a UX standpoint, I do find it slightly odd that it gives you exactly whats on the screen. When downloading the PNG from the dashboard, if your chart is on a really tiny square, then you download a really tiny image. ex:

Bar Chart-2_22_2023, 10_27_31 PM

That's probably what we expect, but I was a little surprised when playing with it. On the plus side though, it captures all your series settings!

@npfitz
Copy link
Contributor

npfitz commented Feb 23, 2023

Looking closer at Ryans screenshot, the 1px gif is not what he is referring to haha. Whoops.

@alxnddr alxnddr merged commit 16f8a52 into master Feb 23, 2023
@alxnddr alxnddr deleted the save-charts-as-pngs branch February 23, 2023 22:37
@iethree
Copy link
Contributor

iethree commented Feb 23, 2023

saving from dashboards works great, but I'm getting 0 byte downloads for downloads directly from questions. the console output leads me to believe it thinks the height is 0px?

just to document, somehow my loom extension was manipulating the DOM in my browser such that it thought that the chart had zero height. It looks like sasha adjusted the css to force it to full height to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

export charts as image
4 participants