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

Fix thumbnails for prominent link to Graphers / Explorers #2893

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

ikesau
Copy link
Member

@ikesau ikesau commented Nov 3, 2023

An idea at a fix for #2870

I really can't see how prominent links were working with plain Grapher URLs before, except I know they were. Something must have been removed at some point. 😓

Anyway, this adds a little fork at the Prominent Link level, to load via the Grapher preview worker instead of via the Image attachments system. It doesn't currently take the query string into account, that but seems like an improvement over it breaking.

Example Archie

{.prominent-link}
url: https://ourworldindata.org/grapher/life-expectancy
{}

{.prominent-link}
url: https://ourworldindata.org/explorers/poverty-explorer
{}

Results

image

@marcelgerber do you think this is an okay way to do this? In my mind this is the same sort of dependency as DATA_API_URL , but maybe there are reasons we don't want to add this?

If we go with this we also might want to look at how BAKED_GRAPHER_EXPORTS_BASE_URL is formed

@ikesau ikesau requested a review from marcelgerber November 3, 2023 19:41
@ikesau ikesau force-pushed the prominent-link-fix branch from ffa7baa to 364686b Compare November 3, 2023 19:42
@ikesau ikesau changed the title Prominent link fix Fix thumbnails for prominent link to Graphers / Explorers Nov 3, 2023
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.

Hm, this is interesting.

Afaict, the only problem that we had before is that <Image filename={BAKED_GRAPHER_EXPORTS_BASE_URL + "/SLUG.svg"} /> didn't work, because Image.ts couldn't find any metadata for that image (makes sense!).
On the other hand, going with <img src={BAKED_GRAPHER_EXPORTS_BASE_URL + "/SLUG.svg"} /> instead works just fine!


As I think we've discussed before, I'm still a bit hesitant to go with the thumbnails worker in more places because while it's working great in most cases, there are some charts (e.g. covid charts) where it fails, and it sometimes takes a few seconds to render a thumbnail...

@ikesau ikesau force-pushed the prominent-link-fix branch 2 times, most recently from 1d43c30 to 6d2226f Compare November 6, 2023 23:16
@ikesau ikesau merged commit c3132c5 into master Nov 7, 2023
25 checks passed
@ikesau ikesau deleted the prominent-link-fix branch November 7, 2023 19:20
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.

2 participants