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

Add grapher image fallback to GDoc pages #3635

Merged
merged 3 commits into from
May 28, 2024
Merged

Conversation

rakyi
Copy link
Contributor

@rakyi rakyi commented May 22, 2024

Restore old functionality by adding image fallback for embedded graphers which will be shown by skipping the dynamic embedding in the following cases:

  • The user has JS disabled
  • The user doesn't have a powerful enough device, see shouldProgressiveEmbed, except on a grapher/data page
    • In this case we also show a note highlighting the possibility of opening the interactive chart on a separate page

WP page:

image

GDoc page:

image

Fixes #2354

@rakyi rakyi requested a review from ikesau May 22, 2024 19:43
@@ -14,3 +14,5 @@ export const POLYFILL_URL: string = `https://cdnjs.cloudflare.com/polyfill/v3/po
)}`

export const DEFAULT_LOCAL_BAKE_DIR = "localBake"

export const GRAPHER_PREVIEW_CLASS = "grapherPreview"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved here to be importable from both client and server.

@owidbot
Copy link
Contributor

owidbot commented May 22, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-gdocs-grapher-fallback

SVG tester:

Number of differences (default views): 4530 (d2becb)
Number of differences (all views): 2744 (6e66b5)

Edited: 2024-05-28 11:16:42 UTC
Execution time: 1.22 seconds

@rakyi rakyi marked this pull request as draft May 22, 2024 20:02
@rakyi rakyi force-pushed the gdocs-grapher-fallback branch 2 times, most recently from ab38742 to bd55d70 Compare May 23, 2024 10:22
@@ -105,7 +111,22 @@ export default function Chart({
border: "0px none",
height: d.height,
}}
/>
>
{isExplorer ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have static images for explorers, and I'm not sure if it's worth it to make it work for them.

@rakyi rakyi force-pushed the gdocs-grapher-fallback branch from bd55d70 to 7587ede Compare May 23, 2024 14:09
@rakyi rakyi marked this pull request as ready for review May 23, 2024 14:24
Restore old functionality by adding image fallback for embedded graphers
which will be shown by skipping the dynamic embedding in the following
cases:

* The user has JS disabled
* The user doesn't have a powerful enough device, see
  `shouldProgressiveEmbed`, except on a grapher/data page
    - In this case we also show a note highlighting the possibility of
      opening the interactive chart on a separate page
@rakyi rakyi force-pushed the gdocs-grapher-fallback branch from 7587ede to a766eaf Compare May 27, 2024 16:57
Copy link
Member

@ikesau ikesau left a comment

Choose a reason for hiding this comment

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

Nice! Once you fix the resolvedUrl thing this'll be good to go. 🙂

I see you've started on the one for datapages, too. If there's a way to fix that that will also make collections work, that'd be a nice bonus, but we haven't really promoted collections at all and I don't think anyone uses them so no worries if you can't get those working (afaict, your changes for them in this PR only affect mobile loading, not no-JS loading)

@@ -0,0 +1,20 @@
import React from "react"

// A modified FontAwesome icon. This is a string to be used outside of React
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what you mean by WordPress content here? Won't we be using this in gdocs baking too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded. Is it better now?

site/gdocs/components/Chart.tsx Outdated Show resolved Hide resolved
@rakyi
Copy link
Contributor Author

rakyi commented May 28, 2024

I didn't even realize collections don't work without JS. Will look into it as well.

@rakyi rakyi merged commit 0a3c640 into master May 28, 2024
21 checks passed
@rakyi rakyi deleted the gdocs-grapher-fallback branch May 28, 2024 11:17
@rakyi
Copy link
Contributor Author

rakyi commented May 28, 2024

Now I'm confused, I thought collections worked without JS when I tried it locally, but then they didn't work on staging, but now it works on prod, e.g. https://ourworldindata.org/collection/top-charts

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.

Gdoc article Grapher embeds don't have SVG fallbacks
3 participants