Implement basic share images #30
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| <CssBaseline /> | ||
| <Story /> | ||
| </ThemeProvider> | ||
| </MetricCatalogProvider> |
There was a problem hiding this comment.
We need the MetricCatalogProvider to make metrics available to components in Storybook (will port this to the template)
| @@ -17,7 +17,8 @@ | |||
| "repl": "yarn run-script scripts/repl/repl.ts", | |||
| "hello": "yarn run-script scripts/hello/index.ts", | |||
| "generate-data-snapshot": "yarn run-script scripts/data/generate-snapshot.ts", | |||
| "create-share-page": "plop share-page" | |||
| "create-share-page": "plop share-page", | |||
| "create-component": "plop component" | |||
There was a problem hiding this comment.
Will we port this plop to the template?
There was a problem hiding this comment.
Yes, actually I didn't really use it, but I will test & port!
| @@ -25,7 +25,7 @@ import { | |||
|
|
|||
| import { RegionSearch } from "@actnowcoalition/actnow.js"; | |||
|
|
|||
| import { regions } from "src/utils/regions"; | |||
| import { regions } from "../../utils/regions"; | |||
There was a problem hiding this comment.
Maybe we should remove "baseUrl": "." from the tsconfig so you can't do this in the first place?
There was a problem hiding this comment.
I'm actually okay with src/utils/regions, but Storybook isn't - I think I need to add src and components as aliases to the Storybook config, but didn't want to get into that on this PR
There was a problem hiding this comment.
Okay, cool. When I tried to do "baseUrl": "src" in act-now-packages, it broke storybook and jest as well as any projects consuming the actnow.js package. I fixed the first 2 but I think the 3rd was not fixable, so I had to revert it and now I feel like baseUrl maybe adds unnecessary complexity. But I don't feel strongly. 😁
|
Can you remove "Close #28" from the PR description if this isn't actually the final PR? |
| } | ||
| } | ||
| }; | ||
| useMutationObserver(ref, handleMutations, { childList: true, subtree: true }); |
There was a problem hiding this comment.
FWIW- Repeating all of this in every share page seems non-ideal. I'm also not entirely understanding how it works (it seems like we are waiting for "act-now-component-loaded" class and then adding a different "screenshot-ready" class; why not just have the screenshot capture use the "act-now-component-loaded" class directly? I may be missing something...)
I'll throw a topic on the standup discussion to discuss with Sean on Monday.
There was a problem hiding this comment.
Yeah, maybe we can create a hook that handles all that in one go
There was a problem hiding this comment.
Yeah, this got really messy and is overcomplicated for most use cases.
The problem that the mutation observer is trying to solve is the situation where you have a share page with two components that use the "act-now-component-loaded" class upon load. If one component typically loads more slowly than the other (e.g. a <MetricUsMap/> vs a <MetricValue/>), then, if we just look for the "act-now-component-loaded" class, the screenshot could be taken after the <MetricValue/> loads but while the <MetricUsMap/> is still gray/loading.
Using the mutation observer to wait until the "slow to load" component is ready sort-of resolves this (it's not a guarantee that the map will always load more slowly), but it is clunky. I think it's worth exploring ways to clean it up
There was a problem hiding this comment.
Not 100% sure if this would work, but maybe we could have components add a act-now-component-loading class while they're loading and a act-now-component-loaded class when they're done loading. And then the screenshot capturer could look for at least one act-now-component-loaded and no act-now-component-loading elements or something.
Anyway, thanks for the details! We can discuss more at standup.
|
With your new Right now there's a bit of whitespace in the screenshots (example) because the |
Part of #28 - Implement share images for the home and location pages.
In this PR, I'm using the same share image for both pages (except for the location name) to test the sharing service first and have the setup ready. The share images look like this:
I also updated the MetricId enum to make it easier to pass the metric as a URL parameter, if we use it to have share images per metric.
Testing
Run the app locally and then go to: