-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
Implement thumbnails for multidimensional data pages #4475
Conversation
8fab58e
to
506271a
Compare
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 0 ✅ Edited: 2025-01-28 11:05:10 UTC |
9287b52
to
ec89e8c
Compare
site/multiDim/MultiDimDataPage.tsx
Outdated
} | ||
// Due to thumbnails not taking into account URL parameters, they are often inaccurate on | ||
// social media. We decided to remove them and use a single thumbnail for all charts. | ||
// See https://github.com/owid/owid-grapher/issues/1086 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? We do show chart previews based on query params
See rewriteMetaTags
So at least one of us must be confused 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we've resolved this, I'll actually test the PR 👍
(Though, I might need a refresher on how to get MDD's running locally 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's pretty cool that we do that. I copy-pasted the existing code from the data page, so I'll remove those comments in both places then. And, it works the same for mdims out of the box:
http://staging-site-mdim-thumbnails/grapher/mdd-demo-poverty?povertyLine=30&metric=share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing mdims locally is quite complicated. You need a relatively fresh copy of the DB. The previews are available at an admin sub-path, e.g. http://localhost:3030/admin/grapher/mdd-demo-poverty
(even if the mdim is not published). Let me know if you'll need help with it.
6e947ba
to
3955aa2
Compare
} | ||
const imageUrl: string = urljoin(baseUrl, "default-grapher-thumbnail.png") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to rebake the page first (since it was missing due to a bug I'm fixing here), but I see it rewritten there:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some ideas on what might be going on with your local dev setup, and it would be easiest if we could have a call to debug it together. Short summary:
-
Make sure you have the latest code and data in the DB
-
To see the normal mdim page at
/grapher
(not the admin preview) you need to setpublished
to1
inmulti_dim_data_pages
and run the migration script, so it propagates to chart configs with:yarn tsx --tsconfig tsconfig.tsx.json devTools/migrateMultiDimConfigs/migrateMultiDimConfigs.ts
(I'll be adding an admin for publishing mdims later this cycle.)
57677d5
to
39ac01f
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huzzah! 🙌
Code looks good and the feature works 😎
We can't use a dynamic thumbnail, because the meta tags are generated during baking. We could switch to dynamic thumbnails if/when we move the Grapher pages to dynamic rendering, i.e Cloudflare functions.
We now rewrite the image meta tags dynamically in Cloudflare functions to take query params into consideration.
This makes it more general and usable also for other routes, such as data download.
39ac01f
to
3c4258a
Compare
We can't use a dynamic thumbnail for social media previews, because the meta tags are generated during baking. We could switch to dynamic thumbnails if/when we move the Grapher pages to dynamic rendering, i.e Cloudflare functions.
Resolves #4456