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: Image download #1931

Merged
merged 33 commits into from
Dec 9, 2024
Merged

feat: Image download #1931

merged 33 commits into from
Dec 9, 2024

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Dec 3, 2024

Contributes to #1928

This PR contains a first implementation of an image download feature, making it possible to download the images in PNG format, hide certain elements or modify the screenshot node on the fly.

Table charts are not possible to be screenshotted as of now.

How to test

  1. Go to this link.
  2. Click on more options button (three dots).
  3. See that there is a new entry (Export PNG) in the popover.
  4. ✅ See that it's possible to download PNG images.
  5. ✅ Change chart types and see that it's possible to use this function with every chart except tables.
  6. ✅ Download an image and see that:
    • every text element is dark-grey,
    • interactive elements were removed from the screenshots (e.g. more options button, drag handles, Details panel, interactive filters, time slider, brush, etc),
    • there's a Created with visualize.admin.ch text at the bottom of the screenshot.
  7. ✅ See that the file name includes date and either chart title or type (in case title is empty).
  8. Open PNG metadata checker (https://raw.pics.io/photo-metadata-viewer) and upload one of the downloaded files.
  9. ✅ See that the file includes Publisher (contact point name and email if exist, cube creator or source otherwise), Dataset (name and version) and Publish URL (if the screenshotted chart was published). Note that the information comes with removed diacritics due to encoding limitations of tEXt chunks.
  10. Add a new chart and switch to the free canvas layout.
  11. ✅ Screenshot smaller charts and see that it works.

⚠️ I've noticed that sometimes there's a bug in Safari when trying to download an image of a map chart, where the canvas is empty. I spent some time investigating but couldn't narrow down the exact issue, I assume it has something to do with internal browser rendering logic. I didn't notice this problem on other browsers.

This would need to be investigated further.

Copy link

vercel bot commented Dec 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 9:05pm

...in order so we don't have to use i18n provider when making a screenshot.
@sosiology
Copy link
Contributor

Thank you for this @bprusinowski LGTM! I tested on Chrome and didn't encounter any issues.
@KerstinFaye please also review.

Copy link
Contributor

@noahonyejese noahonyejese left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 Well done @bprusinowski

app/utils/use-screenshot.ts Show resolved Hide resolved
@@ -403,6 +403,8 @@ export const MapComponent = () => {
initialViewState={defaultViewState}
mapLib={maplibregl}
mapStyle={mapStyle}
// Important so we can take a screenshot of the map
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would say yes, as preserveDrawingBuffer is false by default and can decrease the performance of the map, the comment should make sure that we don't remove it and unknowingly break screenshotting of the maps :)


return componentIds
.map((id) => components.find((component) => component.id === id))
.filter(truthy); // exclude potential joinBy components
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I would also say yes, as I wouldn't know why we need to use truthy for – in this case, we have a connection between using it and joinBy dimensions. Maybe it would be better to have something like

const excludeJoinByComponent = (c: Component) => truthy(c);

but I think a comment should suffice here 👍 It was a bit of a pain to catch all the edge-cases of merging of cubes, I'd rather keep more context in places related to them 🥹

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think comments that describe a "clear" logic should be generally removed, like this:

// Find a component
const component = components.find(c => c.id === id);

but when they are related to other, connected parts of the code or generally are not "obvious", it's okay to use them 👍

package.json Show resolved Hide resolved
@bprusinowski bprusinowski merged commit 3925fc4 into main Dec 9, 2024
6 of 10 checks passed
@bprusinowski bprusinowski deleted the feat/image-download branch December 9, 2024 16:23
@KerstinFaye
Copy link

KerstinFaye commented Dec 10, 2024

The whole flow works perfectly. 🎉 I tested on Chrome. On Safari I was always landing on the search page. Somehow the link did not correctly redirect. Not sure why. But anyways it worked all perfectly.

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.

4 participants