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.

Sign up for GitHub

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

Jump to bottom

🔨 Switch grapher exports to use CF worker #4464

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

🔨 Switch grapher exports to use CF worker #4464

wants to merge 17 commits into from
+332 −720

Conversation

ikesau
Copy link
Member

@ikesau ikesau commented Jan 17, 2025 •
edited
Loading

Resolves #4093 and #3661

Supersedes #3746 and #4099

Changes

  • Switches all references to /exports/ to use our dynamic thumbnails instead
  • Removes code that baked graphers to /exports/
    • Some charts in country profiles will no longer load in the NoJS version of the site because their slugs have updated but formatWordpressPost isn't checking redirects. We could:
      1. Fix this in the code
      2. Update the country profile templates
      3. Not fix it
  • In all thumbnail imgs where applicable, switches the image extension from .svg to .png, to ensure webfonts always show correctly.
  • Removes InteractionNotice

Caveats

  • NoJS country profile pages are still a bit broken, as they don't have the correct entity selected, nor do they take redirects into account

@owidbot
Copy link
Contributor

owidbot commented Jan 17, 2025 •
edited
Loading

Quick links (staging server):

Site Dev Site Preview Admin Wizard Docs

Login: ssh owid@staging-site-remove-grapher-exports

SVG tester:

Number of differences (default views): 770 (7e5427) ❌
Number of differences (all views): 352 (ca1a58) ❌

Edited: 2025-01-23 20:58:24 UTC
Execution time: 1.23 seconds

@ikesau
Copy link
Member Author

ikesau commented Jan 17, 2025

This is blocked by #4450

So I'm going to work on that now

@ikesau ikesau force-pushed the remove-grapher-exports branch from 3f6aeab to d38cba1 Compare January 22, 2025 15:29
ikesau
ikesau commented Jan 22, 2025
View reviewed changes
baker/GrapherImageBaker.tsx
import path from "path"
import sharp from "sharp"
import svgo from "svgo"
Copy link
Member Author

Choose a reason for hiding this comment

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

We still use svgo in the flag updater dev tool

ikesau
ikesau commented Jan 22, 2025
View reviewed changes
site/InteractionNotice.tsx
@@ -1,16 +0,0 @@
export const INTERACTIVE_ICON_SVG = `<svg aria-hidden="true" focusable="false" data-prefix="fas" data-icon="hand-pointer" class="svg-inline--fa fa-hand-pointer fa-w-14" role="img" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 448 617">
Copy link
Member Author

Choose a reason for hiding this comment

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

Deleting InteractionNotice because we weren't using it.

shouldProgressiveEmbed has been hard-coded to true for the last year, so all mobile devices have been embedding charts normally.

Which means that the only time this would render is if JS was disabled, but we were hiding the notice when JS was disabled because it doesn't make sense to explain to the user that clicking on the thumbnail will load an interactive chart in that case 🙃

We could have rewritten shouldProgressiveEmbed, but I think we can circle back to that if/once #4377 is done.

@ikesau ikesau requested a review from marcelgerber January 22, 2025 22:23
@ikesau
Copy link
Member Author

ikesau commented Jan 22, 2025

Some useful pages for testing:

http://staging-site-remove-grapher-exports

http://staging-site-remove-grapher-exports/a-history-of-global-living-conditions

Data page:
http://staging-site-remove-grapher-exports/grapher/life-expectancy

Data page with query params:
http://staging-site-remove-grapher-exports/grapher/life-expectancy?tab=map

Grapher page:
http://staging-site-remove-grapher-exports/grapher/share-extreme-poverty-vs-share-mpi-poor

Grapher page with query params:
http://staging-site-remove-grapher-exports/grapher/share-extreme-poverty-vs-share-mpi-poor?country=IND

Modular topic page:
http://staging-site-remove-grapher-exports/poverty

Linear topic page:
http://staging-site-remove-grapher-exports/age-structure

Country profile page:
http://staging-site-remove-grapher-exports/co2/country/canada

@marcelgerber marcelgerber mentioned this pull request Jan 22, 2025
Open
27 tasks
marcelgerber
marcelgerber requested changes Jan 23, 2025
View reviewed changes
Copy link
Member

@marcelgerber marcelgerber left a comment

Choose a reason for hiding this comment

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

Nice!

You went a bit over the top in MultiEmbedder though, effectively making all non-standalone charts static. Which is good for testing this, but... 😀

baker/formatWordpressPost.tsx Show resolved Hide resolved
ops/buildkite/deploy-content Outdated Show resolved Hide resolved
site/search/ChartHit.tsx Outdated Show resolved Hide resolved
site/multiembedder/MultiEmbedder.tsx Outdated Show resolved Hide resolved
ikesau added 17 commits January 23, 2025 15:51
@ikesau
🔨 remove grapher static exports, replace with CF worker URL
0b5e782
@ikesau
✨ use CF function to update no-JS fallback image on grapher page HTML
78e2e4d
@ikesau
🐛 use query string in formatWordpressPost
2e24535
@ikesau
✨ use <source> in GrapherImage and incorporate into grapher thumbnail…
5c28767 
… CF function
@ikesau
✨ remove-grapher-exports PR feedback
e43494e
@ikesau
✨ remove svg exports straggler code
fbfce20
@ikesau
✨ remove mockSiteRouter svg chart preview
f0ac282
@ikesau
🐛 fix srcSet issue with populateParams
6051397
@ikesau
✨ set width on related-charts list items
b6907b1
@ikesau
🐛 fix rebase
74bad21
@ikesau
✨ remove remaining exports references
fdfa892
@ikesau
✨ update embedded thumbnails with worker when query string params are…
46a071d 
… specified
@ikesau
✨ change linkedChart thumbnail to png
161bfeb
@ikesau
🔨 rename bakeAllChangedGrapherPagesVariablesPngSvgAndDeleteRemovedGra…
36bff85 
…phers
@ikesau
✨ remove InteractionNotice
05a6f79
@ikesau
🔨 fix botched rename
43b8c80
@ikesau
✨ removing static exports PR improvements
14a3108
@ikesau ikesau force-pushed the remove-grapher-exports branch from d2013dd to 14a3108 Compare January 23, 2025 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelgerber marcelgerber marcelgerber requested changes

Assignees

@ikesau ikesau

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Webfonts aren't loading correctly in Grapher SVG thumbnails
3 participants
@ikesau @owidbot @marcelgerber