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

Implement basic share images #30

Merged
merged 4 commits into from
Feb 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions .storybook/preview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,19 @@ import React from "react";
import CssBaseline from "@mui/material/CssBaseline";
import { ThemeProvider } from "@mui/material/styles";

import { MetricCatalogProvider } from "@actnowcoalition/actnow.js";

import theme from "../src/styles/theme";
import { metricCatalog } from "../src/utils/metrics";

// Wraps stories with the MUI Theme provider
const themeDecorator = (Story) => (
<ThemeProvider theme={theme}>
<CssBaseline />
<Story />
</ThemeProvider>
<MetricCatalogProvider metricCatalog={metricCatalog}>
<ThemeProvider theme={theme}>
<CssBaseline />
<Story />
</ThemeProvider>
</MetricCatalogProvider>
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 need the MetricCatalogProvider to make metrics available to components in Storybook (will port this to the template)

);

export const decorators = [themeDecorator];
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

@mikelehen mikelehen Feb 3, 2023

Choose a reason for hiding this comment

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

Will we port this plop to the template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, actually I didn't really use it, but I will test & port!

},
"lint-staged": {
"*.{ts,tsx}": "eslint --max-warnings=0",
Expand Down
66 changes: 65 additions & 1 deletion plopfile.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,41 @@
*/
import _ from "lodash";

const templateComponentMain = prepareTemplate(`
import React from "react";
export interface {{pascalCase name}}Props {
}
export const {{pascalCase name}} = (props: {{pascalCase name}}Props) => {
return <>{{pascalCase name}}</>;
};`);

const templateComponentStories = prepareTemplate(`
import React from "react";
import { ComponentStory, ComponentMeta } from "@storybook/react";
import { {{pascalCase name}} } from ".";
export default {
title: "Components/{{pascalCase name}}",
component: {{pascalCase name}},
} as ComponentMeta<typeof {{pascalCase name}}>;
const Template: ComponentStory<typeof {{pascalCase name}}> = (args) => (
<{{pascalCase name}} {...args} />
);
export const Example = Template.bind({});
Example.args = {};
`);

const templateComponentIndex = prepareTemplate(`
export * from "./{{pascalCase name}}";
`);

const componentBasePath = "src/components";

const templateSharePage = prepareTemplate(`
import { useRef, useState } from "react";
Expand Down Expand Up @@ -59,7 +94,7 @@ const templateSharePageListItem = prepareTemplate(`
</ListItem>
$1`);

export default function (/** @type {import('plop').NodePlopAPI} */ plop) {
function plopConfig(/** @type {import('plop').NodePlopAPI} */ plop) {
const internalPagePath = "src/pages/internal";
plop.setGenerator("share-page", {
description:
Expand Down Expand Up @@ -92,8 +127,37 @@ export default function (/** @type {import('plop').NodePlopAPI} */ plop) {
},
],
});
plop.setGenerator("component", {
description: "Creates a component module with stories, styles and index.",
prompts: [
{
type: "input",
name: "name",
message: "Component name",
},
],
actions: [
{
type: "add",
path: `${componentBasePath}/{{pascalCase name}}/index.ts`,
template: templateComponentIndex,
},
{
type: "add",
path: `${componentBasePath}/{{pascalCase name}}/{{pascalCase name}}.tsx`,
template: templateComponentMain,
},
{
type: "add",
path: `${componentBasePath}/{{pascalCase name}}/{{pascalCase name}}.stories.tsx`,
template: templateComponentStories,
},
],
});
}

export default plopConfig;

function prepareTemplate(srcTemplate) {
return _.trimStart(srcTemplate);
}
2 changes: 1 addition & 1 deletion src/components/AppBar/AppBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {

import { RegionSearch } from "@actnowcoalition/actnow.js";

import { regions } from "src/utils/regions";
import { regions } from "../../utils/regions";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should remove "baseUrl": "." from the tsconfig so you can't do this in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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. 😁


interface AppBarItem {
href: string;
Expand Down
2 changes: 1 addition & 1 deletion src/components/LocationOverview/LocationOverview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const LocationOverview = ({ region }: LocationOverviewProps) => {
return (
<Box display="flex" flexDirection={{ xs: "column", sm: "row" }}>
<Box flex={1}>
<MetricScoreOverview region={region} metric={MetricId.LIFE_LADDER} />
<MetricScoreOverview region={region} metric={MetricId.HAPPINESS} />
<Box mt={4}>
<Typography variant="paragraphLarge">
<ul>
Expand Down
13 changes: 13 additions & 0 deletions src/components/ShareImages/ShareImage.styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { Box } from "@mui/material";

import { styled } from "../../styles";

export const ratio = 16 / 9;
export const width = 900;
export const height = width / ratio;

export const ShareImageContainer = styled(Box)`
padding: ${({ theme }) => theme.spacing(4, 5)};
width: ${width}px;
height: ${height}px;
`;
17 changes: 17 additions & 0 deletions src/components/ShareImages/ShareImageHomepage.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import React from "react";

import { ComponentMeta, ComponentStory } from "@storybook/react";

import { ShareImageHomepage } from ".";

export default {
title: "Components/ShareImageHomepage",
component: ShareImageHomepage,
} as ComponentMeta<typeof ShareImageHomepage>;

const Template: ComponentStory<typeof ShareImageHomepage> = () => (
<ShareImageHomepage />
);

export const Home = Template.bind({});
Home.args = {};
31 changes: 31 additions & 0 deletions src/components/ShareImages/ShareImageHomepage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import React from "react";

import { Box, Stack, Typography } from "@mui/material";

import { MetricWorldMap } from "@actnowcoalition/actnow.js";

import { MetricId } from "../../utils/metrics";
import { regions } from "../../utils/regions";
import { ShareImageContainer } from "./ShareImage.styles";

export const ShareImageHomepage = () => (
<ShareImageContainer sx={{ background: "linear-gradient(#00bfa5, #0091ea)" }}>
<Stack spacing={2} alignItems="center">
<Typography variant="h1" sx={{ mt: 0 }} color="white">
World Happiness Report 2022
</Typography>
<Box
sx={{
width: 700,
backgroundColor: (theme) => theme.palette.common.white,
borderRadius: 2,
}}
>
<MetricWorldMap regionDB={regions} metric={MetricId.HAPPINESS} />
</Box>
<Typography variant="labelLarge" color="white">
By Act Now Coalition
</Typography>
</Stack>
</ShareImageContainer>
);
18 changes: 18 additions & 0 deletions src/components/ShareImages/ShareImageLocation.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from "react";

import { ComponentMeta, ComponentStory } from "@storybook/react";

import { ShareImageLocation } from ".";
import { regions } from "../../utils/regions";

export default {
title: "Components/ShareImageLocation",
component: ShareImageLocation,
} as ComponentMeta<typeof ShareImageLocation>;

const Template: ComponentStory<typeof ShareImageLocation> = () => (
<ShareImageLocation region={regions.findByRegionIdStrict("NOR")} />
);

export const Home = Template.bind({});
Home.args = {};
31 changes: 31 additions & 0 deletions src/components/ShareImages/ShareImageLocation.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import React from "react";

import { Box, Stack, Typography } from "@mui/material";

import { MetricWorldMap, Region } from "@actnowcoalition/actnow.js";

import { MetricId } from "../../utils/metrics";
import { regions } from "../../utils/regions";
import { ShareImageContainer } from "./ShareImage.styles";

export const ShareImageLocation = ({ region }: { region: Region }) => (
<ShareImageContainer sx={{ background: "linear-gradient(#00bfa5, #0091ea)" }}>
<Stack spacing={2} alignItems="center">
<Typography variant="h1" sx={{ mt: 0 }} color="white">
{`Happiness in ${region.shortName}`}
</Typography>
<Box
sx={{
width: 700,
backgroundColor: (theme) => theme.palette.common.white,
borderRadius: 2,
}}
>
<MetricWorldMap regionDB={regions} metric={MetricId.HAPPINESS} />
</Box>
<Typography variant="labelLarge" color="white">
By Act Now Coalition
</Typography>
</Stack>
</ShareImageContainer>
);
2 changes: 2 additions & 0 deletions src/components/ShareImages/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from "./ShareImageHomepage";
export * from "./ShareImageLocation";
6 changes: 3 additions & 3 deletions src/components/Trends/Trends.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const Trends = () => {
);

const { error, data } = useDataForRegionsAndMetrics(regions.all, [
MetricId.LIFE_LADDER,
MetricId.HAPPINESS,
]);

if (error || !data) {
Expand All @@ -38,7 +38,7 @@ export const Trends = () => {

// Sort countries by happiness score, low to high
const countriesByHappiness = sortBy(regions.all, (region) => {
const d = data.metricData(region, MetricId.LIFE_LADDER);
const d = data.metricData(region, MetricId.HAPPINESS);
return d.currentValue as number;
});

Expand Down Expand Up @@ -86,7 +86,7 @@ export const Trends = () => {
metrics={ALL_METRICS}
regions={regions.all}
timePeriods={timePeriods}
initialMetric={MetricId.LIFE_LADDER}
initialMetric={MetricId.HAPPINESS}
initialRegions={option.countries}
height={600}
width={0}
Expand Down
34 changes: 34 additions & 0 deletions src/pages/internal/share-image/homepage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { useRef, useState } from "react";

import { Box } from "@mui/material";
import { NextPage } from "next";

import { useMutationObserver } from "@actnowcoalition/actnow.js";

import { ScreenshotWrapper } from "components/Containers";
import { ShareImageHomepage } from "components/ShareImages";
import { searchDomForClass } from "src/utils/share-pages";

// http://localhost:3000/internal/share-image/homepage
const HomeSharePage: NextPage = () => {
const ref = useRef<Element>(null);

const [isLoaded, setIsLoaded] = useState<boolean>(false);
const handleMutations: MutationCallback = (mutations: MutationRecord[]) => {
for (const mutation of mutations) {
if (mutation.type === "childList") {
searchDomForClass(mutation.target as Element, setIsLoaded);
}
}
};
useMutationObserver(ref, handleMutations, { childList: true, subtree: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, maybe we can create a hook that handles all that in one go

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


return (
<ScreenshotWrapper className="screenshot">
<Box ref={ref} className={isLoaded ? "screenshot-ready" : undefined}>
<ShareImageHomepage />
</Box>
</ScreenshotWrapper>
);
};
export default HomeSharePage;
54 changes: 54 additions & 0 deletions src/pages/internal/share-image/location.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { useRef, useState } from "react";

import { Box } from "@mui/material";
import isEmpty from "lodash/isEmpty";
import { NextPage } from "next";
import { useRouter } from "next/router";

import {
Region,
assert,
useMutationObserver,
} from "@actnowcoalition/actnow.js";

import { regions } from "../../../utils/regions";
import { ScreenshotWrapper } from "components/Containers";
import { ShareImageLocation } from "components/ShareImages";
import { searchDomForClass } from "src/utils/share-pages";

// http://localhost:3000/internal/share-image/location?regionId=CAN
const LocationSharePage: NextPage = () => {
const router = useRouter();
const ref = useRef<Element>(null);

const [isLoaded, setIsLoaded] = useState<boolean>(false);
const handleMutations: MutationCallback = (mutations: MutationRecord[]) => {
for (const mutation of mutations) {
if (mutation.type === "childList") {
searchDomForClass(mutation.target as Element, setIsLoaded);
}
}
};
useMutationObserver(ref, handleMutations, { childList: true, subtree: true });

if (isEmpty(router.query)) {
return (
<span>
Page loading or no query params were provided. Expects params: regionId
</span>
);
}

const { regionId } = router.query;
const region = regions.findByRegionIdStrict(regionId as string);
assert(region instanceof Region, `Region with ID ${regionId} not found`);

return (
<ScreenshotWrapper className="screenshot">
<Box ref={ref} className={isLoaded ? "screenshot-ready" : undefined}>
<ShareImageLocation region={region} />
</Box>
</ScreenshotWrapper>
);
};
export default LocationSharePage;
4 changes: 2 additions & 2 deletions src/screens/Homepage/Homepage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,14 @@ const Homepage: React.FC<{ page: Page }> = ({ page }) => {
}}
>
<AutoWidth>
<MetricWorldMap regionDB={regions} metric={MetricId.LIFE_LADDER} />
<MetricWorldMap regionDB={regions} metric={MetricId.HAPPINESS} />
</AutoWidth>
<MetricLegendThreshold
orientation="horizontal"
height={8}
borderRadius={4}
width={240}
metric={MetricId.LIFE_LADDER}
metric={MetricId.HAPPINESS}
/>
</PageSection>

Expand Down
Loading