From 834295254c89cb85c56b9c472102dfa426139a7a Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 30 Aug 2023 17:57:26 +0100 Subject: [PATCH 01/20] trial using context for renderingTarget --- .../decorators/RenderingTargetDecorator.js | 11 +++++++ .../src/components/ArticleBody.tsx | 4 --- .../components/ArticleHeadline.stories.tsx | 26 --------------- .../src/components/ArticleHeadline.tsx | 7 ---- .../src/components/ArticleMeta.stories.tsx | 13 -------- .../src/components/ArticleMeta.test.tsx | 2 -- .../src/components/ArticleMeta.tsx | 4 --- .../src/components/ArticlePage.tsx | 1 + .../src/components/BylineLink.test.tsx | 3 -- .../src/components/BylineLink.tsx | 16 ++++------ .../src/components/Contributor.test.tsx | 2 -- .../src/components/Contributor.tsx | 16 ++-------- .../src/components/GetMatchNav.importable.tsx | 4 --- .../src/components/HeadlineByline.stories.tsx | 12 ------- .../src/components/HeadlineByline.tsx | 13 +------- .../src/components/RenderingTarget.tsx | 4 +++ .../src/layouts/CommentLayout.tsx | 12 +------ dotcom-rendering/src/layouts/DecideLayout.tsx | 32 ++++++++----------- .../src/layouts/ImmersiveLayout.tsx | 14 +------- .../src/layouts/InteractiveLayout.tsx | 12 +------ dotcom-rendering/src/layouts/LiveLayout.tsx | 14 ++------ .../src/layouts/NewsletterSignupLayout.tsx | 10 +----- .../src/layouts/ShowcaseLayout.tsx | 14 ++------ .../src/layouts/StandardLayout.tsx | 6 +--- dotcom-rendering/src/lib/ArticleRenderer.tsx | 9 +++--- .../src/server/render.article.apps.tsx | 13 +++++--- .../src/server/render.article.web.tsx | 15 +++++---- 27 files changed, 71 insertions(+), 218 deletions(-) create mode 100644 dotcom-rendering/.storybook/decorators/RenderingTargetDecorator.js create mode 100644 dotcom-rendering/src/components/RenderingTarget.tsx diff --git a/dotcom-rendering/.storybook/decorators/RenderingTargetDecorator.js b/dotcom-rendering/.storybook/decorators/RenderingTargetDecorator.js new file mode 100644 index 00000000000..f38d17a97f6 --- /dev/null +++ b/dotcom-rendering/.storybook/decorators/RenderingTargetDecorator.js @@ -0,0 +1,11 @@ +import { RenderingTargetContext } from '../../src/components/RenderingTarget'; + +export const RenderingTargetDecorator = (Story, { args }) => { + return ( + + + + ); +}; + +export default RenderingTargetDecorator; diff --git a/dotcom-rendering/src/components/ArticleBody.tsx b/dotcom-rendering/src/components/ArticleBody.tsx index c04988b07fc..36ee2760922 100644 --- a/dotcom-rendering/src/components/ArticleBody.tsx +++ b/dotcom-rendering/src/components/ArticleBody.tsx @@ -10,7 +10,6 @@ import { revealStyles } from '../lib/revealStyles'; import type { ServerSideTests, Switches } from '../types/config'; import type { TableOfContentsItem } from '../types/frontend'; import type { Palette } from '../types/palette'; -import type { RenderingTarget } from '../types/renderingTarget'; import type { TagType } from '../types/tag'; import { Island } from './Island'; import { RecipeMultiplier } from './RecipeMultiplier.importable'; @@ -47,7 +46,6 @@ type Props = { tableOfContents?: TableOfContentsItem[]; lang?: string; isRightToLeftLang?: boolean; - renderingTarget: RenderingTarget; }; const globalH2Styles = (display: ArticleDisplay) => css` @@ -139,7 +137,6 @@ export const ArticleBody = ({ tableOfContents, lang, isRightToLeftLang = false, - renderingTarget, }: Props) => { const isInteractive = format.design === ArticleDesign.Interactive; const palette = decidePalette(format); @@ -240,7 +237,6 @@ export const ArticleBody = ({ isAdFreeUser={isAdFreeUser} isSensitive={isSensitive} abTests={abTests} - renderingTarget={renderingTarget} /> diff --git a/dotcom-rendering/src/components/ArticleHeadline.stories.tsx b/dotcom-rendering/src/components/ArticleHeadline.stories.tsx index 0bace46f79b..c1f50204551 100644 --- a/dotcom-rendering/src/components/ArticleHeadline.stories.tsx +++ b/dotcom-rendering/src/components/ArticleHeadline.stories.tsx @@ -38,7 +38,6 @@ export const ArticleStory = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -65,7 +64,6 @@ export const Feature = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -98,7 +96,6 @@ export const ShowcaseInterview = () => { tags={[]} webPublicationDateDeprecated="" byline="Byline text" - renderingTarget="Web" /> { tags={[]} webPublicationDateDeprecated="" byline="" - renderingTarget="Web" /> { tags={[]} webPublicationDateDeprecated="" byline="Byline text" - renderingTarget="Web" /> { tags={[]} webPublicationDateDeprecated="" byline="Byline text" - renderingTarget="Web" /> { tags={[]} webPublicationDateDeprecated="" byline="" - renderingTarget="Web" /> { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -350,7 +342,6 @@ export const Analysis = () => { format={format(theme)} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -381,7 +372,6 @@ export const Gallery = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -408,7 +398,6 @@ export const Review = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -435,7 +424,6 @@ export const PhotoEssay = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -462,7 +450,6 @@ export const Explainer = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -489,7 +476,6 @@ export const Quiz = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -516,7 +502,6 @@ export const Recipe = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -543,7 +528,6 @@ export const Immersive = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -570,7 +554,6 @@ export const ImmersiveNoMainMedia = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -602,7 +585,6 @@ export const ImmersiveComment = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -629,7 +611,6 @@ export const Editorial = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -656,7 +637,6 @@ export const MatchReport = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -683,7 +663,6 @@ export const SpecialReport = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -710,7 +689,6 @@ export const SpecialReportAlt = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -737,7 +715,6 @@ export const LiveBlog = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -777,7 +754,6 @@ export const DeadBlog = () => { format={format} tags={[]} webPublicationDateDeprecated="" - renderingTarget="Web" /> @@ -805,7 +781,6 @@ export const ReviewWithoutStars = () => { tags={[]} webPublicationDateDeprecated="" byline="Byline text" - renderingTarget="Web" /> { }, ]} webPublicationDateDeprecated="2020-03-28T07:27:19.000Z" - renderingTarget="Web" /> diff --git a/dotcom-rendering/src/components/ArticleHeadline.tsx b/dotcom-rendering/src/components/ArticleHeadline.tsx index b619be13847..ddf3e82c2c1 100644 --- a/dotcom-rendering/src/components/ArticleHeadline.tsx +++ b/dotcom-rendering/src/components/ArticleHeadline.tsx @@ -13,7 +13,6 @@ import { getAgeWarning } from '../lib/age-warning'; import { decidePalette } from '../lib/decidePalette'; import { getZIndex } from '../lib/getZIndex'; import type { Palette } from '../types/palette'; -import type { RenderingTarget } from '../types/renderingTarget'; import type { TagType } from '../types/tag'; import { AgeWarning } from './AgeWarning'; import { DesignTag } from './DesignTag'; @@ -28,7 +27,6 @@ type Props = { hasStarRating?: boolean; hasAvatar?: boolean; isMatch?: boolean; - renderingTarget: RenderingTarget; }; const topPadding = css` @@ -342,7 +340,6 @@ export const ArticleHeadline = ({ hasStarRating, hasAvatar, isMatch, - renderingTarget, }: Props) => { const palette = decidePalette(format); switch (format.display) { @@ -418,7 +415,6 @@ export const ArticleHeadline = ({ format={format} byline={byline} tags={tags} - renderingTarget={renderingTarget} /> )} @@ -574,7 +570,6 @@ export const ArticleHeadline = ({ format={format} byline={byline} tags={tags} - renderingTarget={renderingTarget} /> )} @@ -665,7 +660,6 @@ export const ArticleHeadline = ({ format={format} byline={byline} tags={tags} - renderingTarget={renderingTarget} /> )} @@ -706,7 +700,6 @@ export const ArticleHeadline = ({ format={format} byline={byline} tags={tags} - renderingTarget={renderingTarget} /> )} diff --git a/dotcom-rendering/src/components/ArticleMeta.stories.tsx b/dotcom-rendering/src/components/ArticleMeta.stories.tsx index 56dd6f14f36..a41e7207f39 100644 --- a/dotcom-rendering/src/components/ArticleMeta.stories.tsx +++ b/dotcom-rendering/src/components/ArticleMeta.stories.tsx @@ -85,7 +85,6 @@ export const ArticleStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -128,7 +127,6 @@ export const BrandingStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -156,7 +154,6 @@ export const FeatureStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -183,7 +180,6 @@ export const FeatureWithMismatchedContributor = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -211,7 +207,6 @@ export const FeatureStoryWithSmallBylineImage = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -238,7 +233,6 @@ export const SpecialReportStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -265,7 +259,6 @@ export const SpecialReportAlt = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -292,7 +285,6 @@ export const CommentStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -319,7 +311,6 @@ export const InterviewStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -346,7 +337,6 @@ export const ImmersiveStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -373,7 +363,6 @@ export const TwoContributorsStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); @@ -402,7 +391,6 @@ export const DeadBlogStory = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ))} @@ -431,7 +419,6 @@ export const Dateline = () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" /> ); diff --git a/dotcom-rendering/src/components/ArticleMeta.test.tsx b/dotcom-rendering/src/components/ArticleMeta.test.tsx index efff27f0944..ddefa7c6874 100644 --- a/dotcom-rendering/src/components/ArticleMeta.test.tsx +++ b/dotcom-rendering/src/components/ArticleMeta.test.tsx @@ -33,7 +33,6 @@ describe('ArticleMeta', () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" />, ); @@ -74,7 +73,6 @@ describe('ArticleMeta', () => { shortUrlId="" ajaxUrl="" showShareCount={true} - renderingTarget="Web" />, ); diff --git a/dotcom-rendering/src/components/ArticleMeta.tsx b/dotcom-rendering/src/components/ArticleMeta.tsx index c44da76f2ff..6ce405a31d6 100644 --- a/dotcom-rendering/src/components/ArticleMeta.tsx +++ b/dotcom-rendering/src/components/ArticleMeta.tsx @@ -14,7 +14,6 @@ import { getSoleContributor } from '../lib/byline'; import { decidePalette } from '../lib/decidePalette'; import type { Branding as BrandingType } from '../types/branding'; import type { Palette } from '../types/palette'; -import type { RenderingTarget } from '../types/renderingTarget'; import type { TagType } from '../types/tag'; import { Avatar } from './Avatar'; import { Branding } from './Branding.importable'; @@ -42,7 +41,6 @@ type Props = { ajaxUrl: string; showShareCount: boolean; messageUs?: MessageUs; - renderingTarget: RenderingTarget; }; const meta = (format: ArticleFormat) => { @@ -315,7 +313,6 @@ export const ArticleMeta = ({ ajaxUrl, showShareCount, messageUs, - renderingTarget, }: Props) => { const soleContributor = getSoleContributor(tags, byline); const authorName = soleContributor?.title ?? 'Author Image'; @@ -380,7 +377,6 @@ export const ArticleMeta = ({ byline={byline} tags={tags} format={format} - renderingTarget={renderingTarget} /> )} {messageUs && diff --git a/dotcom-rendering/src/components/ArticlePage.tsx b/dotcom-rendering/src/components/ArticlePage.tsx index 44835312a09..8bf60c58e4d 100644 --- a/dotcom-rendering/src/components/ArticlePage.tsx +++ b/dotcom-rendering/src/components/ArticlePage.tsx @@ -129,6 +129,7 @@ export const ArticlePage = (props: WebProps | AppProps) => { )} + {renderingTarget === 'Apps' ? ( { design: ArticleDesign.Standard, theme: Pillar.News, }} - renderingTarget="Web" />, ); @@ -145,7 +144,6 @@ describe('BylineLink', () => { design: ArticleDesign.Standard, theme: Pillar.News, }} - renderingTarget="Web" />, ); @@ -181,7 +179,6 @@ describe('BylineLink', () => { design: ArticleDesign.Standard, theme: Pillar.News, }} - renderingTarget="Web" />, ); diff --git a/dotcom-rendering/src/components/BylineLink.tsx b/dotcom-rendering/src/components/BylineLink.tsx index fd489cba84a..55f502657a4 100644 --- a/dotcom-rendering/src/components/BylineLink.tsx +++ b/dotcom-rendering/src/components/BylineLink.tsx @@ -1,19 +1,19 @@ import { ArticleDesign } from '@guardian/libs'; +import { useContext } from 'react'; import { getBylineComponentsFromTokens, getSoleContributor, isContributor, } from '../lib/byline'; -import type { RenderingTarget } from '../types/renderingTarget'; import type { TagType } from '../types/tag'; import { FollowWrapper } from './FollowWrapper.importable'; import { Island } from './Island'; +import { RenderingTargetContext } from './RenderingTarget'; type Props = { byline: string; tags: TagType[]; format: ArticleFormat; - renderingTarget: RenderingTarget; }; const applyCleverOrderingForMatching = (titles: string[]): string[] => { @@ -107,12 +107,7 @@ function removeComma(bylinePart: string) { : bylinePart; } -export const BylineLink = ({ - byline, - tags, - format, - renderingTarget, -}: Props) => { +export const BylineLink = ({ byline, tags, format }: Props) => { const tokens = bylineAsTokens(byline, tags); const soleContributor = getSoleContributor(tags, byline); const hasSoleContributor = !!soleContributor; @@ -137,10 +132,13 @@ export const BylineLink = ({ ); }); + const renderingTargetContext = useContext(RenderingTargetContext); + return ( <> {renderedTokens} - {renderingTarget === 'Apps' && soleContributor !== undefined ? ( + {renderingTargetContext === 'Apps' && + soleContributor !== undefined ? ( { title: 'Observer Design', }, ]} - renderingTarget="Web" />, ); @@ -51,7 +50,6 @@ describe('Contributor', () => { title: 'Observer Design', }, ]} - renderingTarget="Web" />, ); diff --git a/dotcom-rendering/src/components/Contributor.tsx b/dotcom-rendering/src/components/Contributor.tsx index 117bb4358f4..861c5958f77 100644 --- a/dotcom-rendering/src/components/Contributor.tsx +++ b/dotcom-rendering/src/components/Contributor.tsx @@ -6,7 +6,6 @@ import { getSoleContributor } from '../lib/byline'; import { decidePalette } from '../lib/decidePalette'; import TwitterIcon from '../static/icons/twitter.svg'; import type { Palette } from '../types/palette'; -import type { RenderingTarget } from '../types/renderingTarget'; import type { TagType } from '../types/tag'; import { BylineLink } from './BylineLink'; @@ -96,15 +95,9 @@ type Props = { byline: string; tags: TagType[]; format: ArticleFormat; - renderingTarget: RenderingTarget; }; -export const Contributor = ({ - byline, - tags, - format, - renderingTarget, -}: Props) => { +export const Contributor = ({ byline, tags, format }: Props) => { const palette = decidePalette(format); const { twitterHandle } = getSoleContributor(tags, byline) ?? {}; @@ -127,12 +120,7 @@ export const Contributor = ({ bylineColorStyles(palette, format), ]} > - + )} {!!twitterHandle && ( diff --git a/dotcom-rendering/src/components/GetMatchNav.importable.tsx b/dotcom-rendering/src/components/GetMatchNav.importable.tsx index 934e3bff527..095ff645a5f 100644 --- a/dotcom-rendering/src/components/GetMatchNav.importable.tsx +++ b/dotcom-rendering/src/components/GetMatchNav.importable.tsx @@ -3,7 +3,6 @@ import { ArticleDesign } from '@guardian/libs'; import { from } from '@guardian/source-foundations'; import type { SWRConfiguration } from 'swr'; import { useApi } from '../lib/useApi'; -import type { RenderingTarget } from '../types/renderingTarget'; import type { TagType } from '../types/tag'; import { ArticleHeadline } from './ArticleHeadline'; import { cleanTeamData } from './GetMatchStats.importable'; @@ -16,7 +15,6 @@ type Props = { format: ArticleFormat; tags: TagType[]; webPublicationDateDeprecated: string; - renderingTarget: RenderingTarget; }; const Loading = () => ; @@ -39,7 +37,6 @@ export const GetMatchNav = ({ format, tags, webPublicationDateDeprecated, - renderingTarget, }: Props) => { const options: SWRConfiguration = { errorRetryCount: 1 }; // If this blog is live then poll for new stats @@ -82,7 +79,6 @@ export const GetMatchNav = ({ webPublicationDateDeprecated } isMatch={true} - renderingTarget={renderingTarget} /> ); diff --git a/dotcom-rendering/src/components/HeadlineByline.stories.tsx b/dotcom-rendering/src/components/HeadlineByline.stories.tsx index 776e406f385..7edef52b17e 100644 --- a/dotcom-rendering/src/components/HeadlineByline.stories.tsx +++ b/dotcom-rendering/src/components/HeadlineByline.stories.tsx @@ -22,7 +22,6 @@ export const interviewStory = () => { }} byline="Jane Smith" tags={[]} - renderingTarget="Web" /> ); }; @@ -38,7 +37,6 @@ export const commentStory = () => { }} byline="Jane Smith" tags={[]} - renderingTarget="Web" /> ); }; @@ -54,7 +52,6 @@ export const specialStory = () => { }} byline="Jane Smith" tags={[]} - renderingTarget="Web" /> ); }; @@ -80,7 +77,6 @@ export const commentWithBylineImageStory = () => { 'https://i.guim.co.uk/img/uploads/2018/01/10/Marina_Hyde,_L.png?width=300&quality=85&auto=format&fit=max&s=6476202195914952e48ef41aadb116ff', }, ]} - renderingTarget="Web" /> ); }; @@ -102,7 +98,6 @@ export const immersiveStory = () => { title: 'Jane Smith', }, ]} - renderingTarget="Web" /> ); }; @@ -130,7 +125,6 @@ export const ImmersiveComment = () => { title: 'Jane Smith', }, ]} - renderingTarget="Web" /> ); @@ -163,7 +157,6 @@ export const MultipleStory = () => { title: 'Nae Bevan', }, ]} - renderingTarget="Web" /> ); }; @@ -190,7 +183,6 @@ export const MultipleDuplicateStory = () => { title: 'Duncan Campbell', }, ]} - renderingTarget="Web" /> ); }; @@ -207,7 +199,6 @@ export const noBylineStory = () => { }} byline="" tags={[]} - renderingTarget="Web" /> ); }; @@ -229,7 +220,6 @@ export const LabsImmersive = () => { title: 'Jane Smith', }, ]} - renderingTarget="Web" /> ); }; @@ -251,7 +241,6 @@ export const LabsComment = () => { title: 'Jane Smith', }, ]} - renderingTarget="Web" /> ); }; @@ -273,7 +262,6 @@ export const LabsInterview = () => { title: 'Jane Smith', }, ]} - renderingTarget="Web" /> ); }; diff --git a/dotcom-rendering/src/components/HeadlineByline.tsx b/dotcom-rendering/src/components/HeadlineByline.tsx index 0b85d5baf7b..d393eb56a91 100644 --- a/dotcom-rendering/src/components/HeadlineByline.tsx +++ b/dotcom-rendering/src/components/HeadlineByline.tsx @@ -12,7 +12,6 @@ import { import { getSoleContributor } from '../lib/byline'; import { decidePalette } from '../lib/decidePalette'; import type { Palette } from '../types/palette'; -import type { RenderingTarget } from '../types/renderingTarget'; import type { TagType } from '../types/tag'; import { BylineLink } from './BylineLink'; @@ -145,15 +144,9 @@ type Props = { format: ArticleFormat; byline: string; tags: TagType[]; - renderingTarget: RenderingTarget; }; -export const HeadlineByline = ({ - format, - byline, - tags, - renderingTarget, -}: Props) => { +export const HeadlineByline = ({ format, byline, tags }: Props) => { if (byline === '') { return null; } @@ -172,7 +165,6 @@ export const HeadlineByline = ({ byline={byline} tags={tags} format={format} - renderingTarget={renderingTarget} /> @@ -190,7 +182,6 @@ export const HeadlineByline = ({ byline={byline} tags={tags} format={format} - renderingTarget={renderingTarget} /> @@ -210,7 +201,6 @@ export const HeadlineByline = ({ byline={byline} tags={tags} format={format} - renderingTarget={renderingTarget} /> @@ -228,7 +218,6 @@ export const HeadlineByline = ({ byline={byline} tags={tags} format={format} - renderingTarget={renderingTarget} /> diff --git a/dotcom-rendering/src/components/RenderingTarget.tsx b/dotcom-rendering/src/components/RenderingTarget.tsx new file mode 100644 index 00000000000..eb7e693c9bc --- /dev/null +++ b/dotcom-rendering/src/components/RenderingTarget.tsx @@ -0,0 +1,4 @@ +import { createContext } from 'react'; +import type { RenderingTarget } from '../types/renderingTarget'; + +export const RenderingTargetContext = createContext('Web'); diff --git a/dotcom-rendering/src/layouts/CommentLayout.tsx b/dotcom-rendering/src/layouts/CommentLayout.tsx index 4d178410fd2..d4672c6a8d2 100644 --- a/dotcom-rendering/src/layouts/CommentLayout.tsx +++ b/dotcom-rendering/src/layouts/CommentLayout.tsx @@ -47,7 +47,6 @@ import { decideTrail } from '../lib/decideTrail'; import { parse } from '../lib/slot-machine-flags'; import type { NavType } from '../model/extract-nav'; import type { FEArticleType } from '../types/frontend'; -import type { RenderingTarget } from '../types/renderingTarget'; import { BannerWrapper, SendToBack, Stuck } from './lib/stickiness'; const StandardGrid = ({ @@ -262,15 +261,9 @@ interface Props { article: FEArticleType; NAV: NavType; format: ArticleFormat; - renderingTarget: RenderingTarget; } -export const CommentLayout = ({ - article, - NAV, - format, - renderingTarget, -}: Props) => { +export const CommentLayout = ({ article, NAV, format }: Props) => { const { config: { isPaidContent, host }, } = article; @@ -490,7 +483,6 @@ export const CommentLayout = ({ 'number' } hasAvatar={!!avatarUrl} - renderingTarget={renderingTarget} /> {/* BOTTOM */}
@@ -559,7 +551,6 @@ export const CommentLayout = ({ !!article.config.switches .serverShareCounts } - renderingTarget={renderingTarget} />
@@ -600,7 +591,6 @@ export const CommentLayout = ({ isRightToLeftLang={ article.isRightToLeftLang } - renderingTarget={renderingTarget} /> {showBodyEndSlot && ( diff --git a/dotcom-rendering/src/layouts/DecideLayout.tsx b/dotcom-rendering/src/layouts/DecideLayout.tsx index 45bd1f64ead..5279dfcdf07 100644 --- a/dotcom-rendering/src/layouts/DecideLayout.tsx +++ b/dotcom-rendering/src/layouts/DecideLayout.tsx @@ -23,17 +23,11 @@ interface AppProps extends BaseProps { } interface WebProps extends BaseProps { - renderingTarget: 'Web'; NAV: NavType; + renderingTarget: 'Web'; } -const DecideLayoutApps = ({ - article, - format, -}: { - article: FEArticleType; - format: ArticleFormat; -}) => { +const DecideLayoutApps = ({ article, format, renderingTarget }: AppProps) => { const notSupported = 
Not supported
; switch (format.display) { case ArticleDisplay.Standard: { @@ -43,7 +37,7 @@ const DecideLayoutApps = ({ ); case ArticleDesign.LiveBlog: @@ -52,7 +46,7 @@ const DecideLayoutApps = ({ ); default: @@ -92,7 +86,6 @@ const DecideLayoutWeb = ({ article={article} NAV={NAV} format={format} - renderingTarget={renderingTarget} /> ); } @@ -119,7 +112,6 @@ const DecideLayoutWeb = ({ article={article} NAV={NAV} format={format} - renderingTarget={renderingTarget} /> ); default: @@ -128,7 +120,6 @@ const DecideLayoutWeb = ({ article={article} NAV={NAV} format={format} - renderingTarget={renderingTarget} /> ); } @@ -142,7 +133,6 @@ const DecideLayoutWeb = ({ article={article} NAV={NAV} format={format} - renderingTarget={renderingTarget} /> ); case ArticleDesign.FullPageInteractive: { @@ -161,7 +151,7 @@ const DecideLayoutWeb = ({ article={article} NAV={NAV} format={format} - renderingTarget="Web" + renderingTarget={renderingTarget} /> ); case ArticleDesign.Comment: @@ -172,7 +162,6 @@ const DecideLayoutWeb = ({ article={article} NAV={NAV} format={format} - renderingTarget={renderingTarget} /> ); case ArticleDesign.NewsletterSignup: @@ -181,7 +170,6 @@ const DecideLayoutWeb = ({ article={article} NAV={NAV} format={format} - renderingTarget={renderingTarget} /> ); default: @@ -198,12 +186,18 @@ const DecideLayoutWeb = ({ } }; -export const DecideLayout = (props: AppProps | WebProps) => { +export const DecideLayout = (props: WebProps | AppProps) => { const { article, format, renderingTarget } = props; switch (renderingTarget) { case 'Apps': - return ; + return ( + + ); case 'Web': return ( ( @@ -184,7 +183,6 @@ interface Props { article: FEArticleType; NAV: NavType; format: ArticleFormat; - renderingTarget: RenderingTarget; } const decideCaption = (mainMedia: FEElement | undefined): string => { @@ -243,12 +241,7 @@ const Box = ({ ); -export const ImmersiveLayout = ({ - article, - NAV, - format, - renderingTarget, -}: Props) => { +export const ImmersiveLayout = ({ article, NAV, format }: Props) => { const { config: { isPaidContent, host }, } = article; @@ -460,7 +453,6 @@ export const ImmersiveLayout = ({ hasStarRating={ article.starRating !== undefined } - renderingTarget={renderingTarget} /> @@ -539,7 +531,6 @@ export const ImmersiveLayout = ({ typeof article.starRating === 'number' } - renderingTarget={renderingTarget} /> )} @@ -557,7 +548,6 @@ export const ImmersiveLayout = ({ format={format} tags={article.tags} byline={article.byline} - renderingTarget={renderingTarget} /> )} @@ -609,7 +599,6 @@ export const ImmersiveLayout = ({ !!article.config.switches .serverShareCounts } - renderingTarget={renderingTarget} /> @@ -647,7 +636,6 @@ export const ImmersiveLayout = ({ isRightToLeftLang={ article.isRightToLeftLang } - renderingTarget={renderingTarget} /> {showBodyEndSlot && ( diff --git a/dotcom-rendering/src/layouts/InteractiveLayout.tsx b/dotcom-rendering/src/layouts/InteractiveLayout.tsx index 187659b6a22..29f4e95d920 100644 --- a/dotcom-rendering/src/layouts/InteractiveLayout.tsx +++ b/dotcom-rendering/src/layouts/InteractiveLayout.tsx @@ -48,7 +48,6 @@ import { decidePalette } from '../lib/decidePalette'; import { decideTrail } from '../lib/decideTrail'; import type { NavType } from '../model/extract-nav'; import type { FEArticleType } from '../types/frontend'; -import type { RenderingTarget } from '../types/renderingTarget'; import { interactiveGlobalStyles, interactiveLegacyClasses, @@ -204,15 +203,9 @@ interface Props { article: FEArticleType; NAV: NavType; format: ArticleFormat; - renderingTarget: RenderingTarget; } -export const InteractiveLayout = ({ - article, - NAV, - format, - renderingTarget, -}: Props) => { +export const InteractiveLayout = ({ article, NAV, format }: Props) => { const { config: { isPaidContent, host }, } = article; @@ -439,7 +432,6 @@ export const InteractiveLayout = ({ typeof article.starRating === 'number' } - renderingTarget={renderingTarget} /> {article.starRating !== undefined ? ( @@ -492,7 +484,6 @@ export const InteractiveLayout = ({ !!article.config.switches .serverShareCounts } - renderingTarget={renderingTarget} /> @@ -531,7 +522,6 @@ export const InteractiveLayout = ({ isRightToLeftLang={ article.isRightToLeftLang } - renderingTarget={renderingTarget} /> diff --git a/dotcom-rendering/src/layouts/LiveLayout.tsx b/dotcom-rendering/src/layouts/LiveLayout.tsx index 6fda765a71a..7147c54c8ed 100644 --- a/dotcom-rendering/src/layouts/LiveLayout.tsx +++ b/dotcom-rendering/src/layouts/LiveLayout.tsx @@ -61,6 +61,7 @@ import { decideTrail } from '../lib/decideTrail'; import { getZIndex } from '../lib/getZIndex'; import type { NavType } from '../model/extract-nav'; import type { FEArticleType } from '../types/frontend'; +import type { RenderingTarget } from '../types/renderingTarget'; import { BannerWrapper, SendToBack, Stuck } from './lib/stickiness'; const HeadlineGrid = ({ children }: { children: React.ReactNode }) => ( @@ -244,6 +245,7 @@ const paddingBody = css` interface BaseProps { article: FEArticleType; format: ArticleFormat; + renderingTarget: RenderingTarget; } interface AppsProps extends BaseProps { @@ -251,8 +253,8 @@ interface AppsProps extends BaseProps { } interface WebProps extends BaseProps { - renderingTarget: 'Web'; NAV: NavType; + renderingTarget: 'Web'; } export const LiveLayout = (props: WebProps | AppsProps) => { @@ -461,7 +463,6 @@ export const LiveLayout = (props: WebProps | AppsProps) => { webPublicationDateDeprecated={ article.webPublicationDateDeprecated } - renderingTarget={renderingTarget} /> @@ -498,7 +499,6 @@ export const LiveLayout = (props: WebProps | AppsProps) => { typeof article.starRating === 'number' } - renderingTarget={renderingTarget} /> )} @@ -585,7 +585,6 @@ export const LiveLayout = (props: WebProps | AppsProps) => { .serverShareCounts } messageUs={article.messageUs} - renderingTarget={renderingTarget} /> @@ -750,7 +749,6 @@ export const LiveLayout = (props: WebProps | AppsProps) => { .serverShareCounts } messageUs={article.messageUs} - renderingTarget={renderingTarget} /> @@ -904,9 +902,6 @@ export const LiveLayout = (props: WebProps | AppsProps) => { .serverSideLiveblogInlineAdsVariant === 'variant' } - renderingTarget={ - renderingTarget - } /> {pagination.totalPages > 1 && ( { isRightToLeftLang={ article.isRightToLeftLang } - renderingTarget={ - renderingTarget - } /> {pagination.totalPages > 1 && ( : undefined, ); -export const NewsletterSignupLayout = ({ - article, - NAV, - format, - renderingTarget, -}: Props) => { +export const NewsletterSignupLayout = ({ article, NAV, format }: Props) => { const { promotedNewsletter, config: { host }, @@ -403,7 +396,6 @@ export const NewsletterSignupLayout = ({ webPublicationDateDeprecated={ article.webPublicationDateDeprecated } - renderingTarget={renderingTarget} /> { +export const ShowcaseLayout = ({ article, NAV, format }: Props) => { const { config: { isPaidContent, host }, } = article; @@ -578,7 +571,6 @@ export const ShowcaseLayout = ({ hasStarRating={ article.starRating !== undefined } - renderingTarget={renderingTarget} /> @@ -623,7 +615,6 @@ export const ShowcaseLayout = ({ !!article.config.switches .serverShareCounts } - renderingTarget={renderingTarget} /> @@ -661,7 +652,6 @@ export const ShowcaseLayout = ({ isRightToLeftLang={ article.isRightToLeftLang } - renderingTarget={renderingTarget} /> {showBodyEndSlot && ( diff --git a/dotcom-rendering/src/layouts/StandardLayout.tsx b/dotcom-rendering/src/layouts/StandardLayout.tsx index d5c81352b41..d5b22b41ceb 100644 --- a/dotcom-rendering/src/layouts/StandardLayout.tsx +++ b/dotcom-rendering/src/layouts/StandardLayout.tsx @@ -340,7 +340,7 @@ export const StandardLayout = (props: WebProps | AppProps) => { return ( <> - {renderingTarget === 'Web' && ( + {isWeb && (
{renderAds && ( @@ -502,7 +502,6 @@ export const StandardLayout = (props: WebProps | AppProps) => { webPublicationDateDeprecated={ article.webPublicationDateDeprecated } - renderingTarget={renderingTarget} /> )} @@ -569,7 +568,6 @@ export const StandardLayout = (props: WebProps | AppProps) => { hasStarRating={ typeof article.starRating === 'number' } - renderingTarget={renderingTarget} />
@@ -628,7 +626,6 @@ export const StandardLayout = (props: WebProps | AppProps) => { !!article.config.switches .serverShareCounts } - renderingTarget={renderingTarget} /> @@ -667,7 +664,6 @@ export const StandardLayout = (props: WebProps | AppProps) => { isRightToLeftLang={ article.isRightToLeftLang } - renderingTarget={renderingTarget} /> {format.design === ArticleDesign.MatchReport && !!footballMatchUrl && ( diff --git a/dotcom-rendering/src/lib/ArticleRenderer.tsx b/dotcom-rendering/src/lib/ArticleRenderer.tsx index 6ccda77ab93..cadd52333b5 100644 --- a/dotcom-rendering/src/lib/ArticleRenderer.tsx +++ b/dotcom-rendering/src/lib/ArticleRenderer.tsx @@ -1,11 +1,12 @@ import { css } from '@emotion/react'; import type { ArticleFormat } from '@guardian/libs'; import { ArticleDesign } from '@guardian/libs'; +import { useContext } from 'react'; import { adContainerStyles } from '../components/AdSlot'; +import { RenderingTargetContext } from '../components/RenderingTarget'; import { interactiveLegacyClasses } from '../layouts/lib/interactiveLegacyStyling'; import type { ServerSideTests, Switches } from '../types/config'; import type { FEElement } from '../types/content'; -import type { RenderingTarget } from '../types/renderingTarget'; import type { TagType } from '../types/tag'; import { RenderArticleElement } from './renderElement'; import { withSignInGateSlot } from './withSignInGateSlot'; @@ -42,7 +43,6 @@ type Props = { isAdFreeUser: boolean; isSensitive: boolean; abTests?: ServerSideTests; - renderingTarget: RenderingTarget; }; export const ArticleRenderer = ({ @@ -63,7 +63,6 @@ export const ArticleRenderer = ({ isSensitive, isDev, abTests, - renderingTarget, }: Props) => { const renderedElements = elements.map((element, index) => { return ( @@ -86,6 +85,8 @@ export const ArticleRenderer = ({ ); }); + const renderingTargetContext = useContext(RenderingTargetContext); + // const cleanedElements = elements.map(element => // 'html' in element ? { ...element, html: clean(element.html) } : element, // ); @@ -105,7 +106,7 @@ export const ArticleRenderer = ({ ].join(' ')} css={[adStylesDynamic, commercialPosition]} > - {renderingTarget === 'Apps' + {renderingTargetContext === 'Apps' ? renderedElements : /* Insert the placeholder for the sign in gate on the 2nd article element */ withSignInGateSlot({ diff --git a/dotcom-rendering/src/server/render.article.apps.tsx b/dotcom-rendering/src/server/render.article.apps.tsx index 30f8faa4698..7b8dc84431b 100644 --- a/dotcom-rendering/src/server/render.article.apps.tsx +++ b/dotcom-rendering/src/server/render.article.apps.tsx @@ -1,5 +1,6 @@ import { isString } from '@guardian/libs'; import { ArticlePage } from '../components/ArticlePage'; +import { RenderingTargetContext } from '../components/RenderingTarget'; import { generateScriptTags, getPathFromManifest } from '../lib/assets'; import { decideFormat } from '../lib/decideFormat'; import { renderToStringWithEmotion } from '../lib/emotion'; @@ -16,11 +17,13 @@ export const renderArticle = ( const format: ArticleFormat = decideFormat(article.format); const { html, extractedCss } = renderToStringWithEmotion( - , + + + , ); const clientScripts = [getPathFromManifest('apps', 'index.js')]; diff --git a/dotcom-rendering/src/server/render.article.web.tsx b/dotcom-rendering/src/server/render.article.web.tsx index 4ab84f7d7f2..39050289e28 100644 --- a/dotcom-rendering/src/server/render.article.web.tsx +++ b/dotcom-rendering/src/server/render.article.web.tsx @@ -2,6 +2,7 @@ import { ArticleDesign, isString, Pillar } from '@guardian/libs'; import { ArticlePage } from '../components/ArticlePage'; import { isAmpSupported } from '../components/Elements.amp'; import { KeyEventsContainer } from '../components/KeyEventsContainer'; +import { RenderingTargetContext } from '../components/RenderingTarget'; import { ASSET_ORIGIN, generateScriptTags, @@ -48,12 +49,14 @@ export const renderHtml = ({ const format: ArticleFormat = decideFormat(article.format); const { html, extractedCss } = renderToStringWithEmotion( - , + + + , ); // We want to only insert script tags for the elements or main media elements on this page view From a2a5b543542d3b82ba38aa04fd9a54d7ce2cf993 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Thu, 31 Aug 2023 17:27:17 +0100 Subject: [PATCH 02/20] refactor: one context to rule them all --- .../decorators/RenderingTargetDecorator.js | 11 -------- .../decorators/renderingContextDecorator.js | 19 ++++++++++++++ dotcom-rendering/.storybook/preview.js | 2 ++ .../src/components/ArticleMeta.stories.tsx | 26 +++++++++++++++++++ .../src/components/BylineLink.tsx | 7 +++-- .../src/components/RenderingContext.tsx | 15 +++++++++++ .../src/components/RenderingTarget.tsx | 4 --- dotcom-rendering/src/lib/ArticleRenderer.tsx | 6 ++--- .../src/server/render.article.apps.tsx | 9 ++++--- .../src/server/render.article.web.tsx | 9 ++++--- .../src/types/renderingContext.ts | 10 +++++++ 11 files changed, 90 insertions(+), 28 deletions(-) delete mode 100644 dotcom-rendering/.storybook/decorators/RenderingTargetDecorator.js create mode 100644 dotcom-rendering/.storybook/decorators/renderingContextDecorator.js create mode 100644 dotcom-rendering/src/components/RenderingContext.tsx delete mode 100644 dotcom-rendering/src/components/RenderingTarget.tsx create mode 100644 dotcom-rendering/src/types/renderingContext.ts diff --git a/dotcom-rendering/.storybook/decorators/RenderingTargetDecorator.js b/dotcom-rendering/.storybook/decorators/RenderingTargetDecorator.js deleted file mode 100644 index f38d17a97f6..00000000000 --- a/dotcom-rendering/.storybook/decorators/RenderingTargetDecorator.js +++ /dev/null @@ -1,11 +0,0 @@ -import { RenderingTargetContext } from '../../src/components/RenderingTarget'; - -export const RenderingTargetDecorator = (Story, { args }) => { - return ( - - - - ); -}; - -export default RenderingTargetDecorator; diff --git a/dotcom-rendering/.storybook/decorators/renderingContextDecorator.js b/dotcom-rendering/.storybook/decorators/renderingContextDecorator.js new file mode 100644 index 00000000000..e95426c7cee --- /dev/null +++ b/dotcom-rendering/.storybook/decorators/renderingContextDecorator.js @@ -0,0 +1,19 @@ +import { RenderingContext } from '../../src/components/RenderingContext'; + +const defaultContext = { target: 'Web' }; + +export const RenderingContextDecorator = ( + Story, + { args: { renderingContext } }, +) => { + const context = { ...defaultContext, ...renderingContext }; + + // For easy debugging + console.log('Storybook rendering context: \n', context); + + return ( + + + + ); +}; diff --git a/dotcom-rendering/.storybook/preview.js b/dotcom-rendering/.storybook/preview.js index 309fa5fb131..6d6c28b5c11 100644 --- a/dotcom-rendering/.storybook/preview.js +++ b/dotcom-rendering/.storybook/preview.js @@ -12,6 +12,7 @@ import { Lazy } from '../src/components/Lazy'; import { Picture } from '../src/components/Picture'; import { mockRESTCalls } from '../src/lib/mockRESTCalls'; import { setABTests } from '../src/lib/useAB'; +import { RenderingContextDecorator } from './decorators/renderingContextDecorator'; // Prevent components being lazy rendered when we're taking Chromatic snapshots Lazy.disabled = isChromatic(); @@ -139,6 +140,7 @@ const guardianViewports = { /** @type {import('@storybook/react').Preview} */ export default { decorators: [ + RenderingContextDecorator, (Story) => { storage.local.clear(); return Story(); diff --git a/dotcom-rendering/src/components/ArticleMeta.stories.tsx b/dotcom-rendering/src/components/ArticleMeta.stories.tsx index a41e7207f39..f484e63228b 100644 --- a/dotcom-rendering/src/components/ArticleMeta.stories.tsx +++ b/dotcom-rendering/src/components/ArticleMeta.stories.tsx @@ -90,6 +90,32 @@ export const ArticleStory = () => { ); }; +export const ArticleAppsStory = () => { + return ( + + + + ); +}; +ArticleAppsStory.args = { renderingContext: { target: 'Apps' } }; + export const BrandingStory = () => { return ( diff --git a/dotcom-rendering/src/components/BylineLink.tsx b/dotcom-rendering/src/components/BylineLink.tsx index 55f502657a4..204591c5325 100644 --- a/dotcom-rendering/src/components/BylineLink.tsx +++ b/dotcom-rendering/src/components/BylineLink.tsx @@ -8,7 +8,7 @@ import { import type { TagType } from '../types/tag'; import { FollowWrapper } from './FollowWrapper.importable'; import { Island } from './Island'; -import { RenderingTargetContext } from './RenderingTarget'; +import { RenderingContext } from './RenderingContext'; type Props = { byline: string; @@ -132,13 +132,12 @@ export const BylineLink = ({ byline, tags, format }: Props) => { ); }); - const renderingTargetContext = useContext(RenderingTargetContext); + const { target } = useContext(RenderingContext); return ( <> {renderedTokens} - {renderingTargetContext === 'Apps' && - soleContributor !== undefined ? ( + {target === 'Apps' && soleContributor !== undefined ? ( (defaultContext); diff --git a/dotcom-rendering/src/components/RenderingTarget.tsx b/dotcom-rendering/src/components/RenderingTarget.tsx deleted file mode 100644 index eb7e693c9bc..00000000000 --- a/dotcom-rendering/src/components/RenderingTarget.tsx +++ /dev/null @@ -1,4 +0,0 @@ -import { createContext } from 'react'; -import type { RenderingTarget } from '../types/renderingTarget'; - -export const RenderingTargetContext = createContext('Web'); diff --git a/dotcom-rendering/src/lib/ArticleRenderer.tsx b/dotcom-rendering/src/lib/ArticleRenderer.tsx index cadd52333b5..95c268f88d6 100644 --- a/dotcom-rendering/src/lib/ArticleRenderer.tsx +++ b/dotcom-rendering/src/lib/ArticleRenderer.tsx @@ -3,7 +3,7 @@ import type { ArticleFormat } from '@guardian/libs'; import { ArticleDesign } from '@guardian/libs'; import { useContext } from 'react'; import { adContainerStyles } from '../components/AdSlot'; -import { RenderingTargetContext } from '../components/RenderingTarget'; +import { RenderingContext } from '../components/RenderingContext'; import { interactiveLegacyClasses } from '../layouts/lib/interactiveLegacyStyling'; import type { ServerSideTests, Switches } from '../types/config'; import type { FEElement } from '../types/content'; @@ -85,7 +85,7 @@ export const ArticleRenderer = ({ ); }); - const renderingTargetContext = useContext(RenderingTargetContext); + const { target } = useContext(RenderingContext); // const cleanedElements = elements.map(element => // 'html' in element ? { ...element, html: clean(element.html) } : element, @@ -106,7 +106,7 @@ export const ArticleRenderer = ({ ].join(' ')} css={[adStylesDynamic, commercialPosition]} > - {renderingTargetContext === 'Apps' + {target === 'Apps' ? renderedElements : /* Insert the placeholder for the sign in gate on the 2nd article element */ withSignInGateSlot({ diff --git a/dotcom-rendering/src/server/render.article.apps.tsx b/dotcom-rendering/src/server/render.article.apps.tsx index 7b8dc84431b..44c37db7c75 100644 --- a/dotcom-rendering/src/server/render.article.apps.tsx +++ b/dotcom-rendering/src/server/render.article.apps.tsx @@ -1,11 +1,12 @@ import { isString } from '@guardian/libs'; import { ArticlePage } from '../components/ArticlePage'; -import { RenderingTargetContext } from '../components/RenderingTarget'; +import { RenderingContext } from '../components/RenderingContext'; import { generateScriptTags, getPathFromManifest } from '../lib/assets'; import { decideFormat } from '../lib/decideFormat'; import { renderToStringWithEmotion } from '../lib/emotion'; import { createGuardian } from '../model/guardian'; import type { FEArticleType } from '../types/frontend'; +import type { RenderingContextType } from '../types/renderingContext'; import { htmlPageTemplate } from './htmlPageTemplate'; export const renderArticle = ( @@ -16,14 +17,16 @@ export const renderArticle = ( } => { const format: ArticleFormat = decideFormat(article.format); + const renderingContext: RenderingContextType = { target: 'Apps' }; + const { html, extractedCss } = renderToStringWithEmotion( - + - , + , ); const clientScripts = [getPathFromManifest('apps', 'index.js')]; diff --git a/dotcom-rendering/src/server/render.article.web.tsx b/dotcom-rendering/src/server/render.article.web.tsx index 39050289e28..856c02c3b5f 100644 --- a/dotcom-rendering/src/server/render.article.web.tsx +++ b/dotcom-rendering/src/server/render.article.web.tsx @@ -2,7 +2,7 @@ import { ArticleDesign, isString, Pillar } from '@guardian/libs'; import { ArticlePage } from '../components/ArticlePage'; import { isAmpSupported } from '../components/Elements.amp'; import { KeyEventsContainer } from '../components/KeyEventsContainer'; -import { RenderingTargetContext } from '../components/RenderingTarget'; +import { RenderingContext } from '../components/RenderingContext'; import { ASSET_ORIGIN, generateScriptTags, @@ -21,6 +21,7 @@ import { extractNAV } from '../model/extract-nav'; import { createGuardian as createWindowGuardian } from '../model/guardian'; import type { FEElement } from '../types/content'; import type { FEArticleType, FEBlocksRequest } from '../types/frontend'; +import type { RenderingContextType } from '../types/renderingContext'; import type { TagType } from '../types/tag'; import { htmlPageTemplate } from './htmlPageTemplate'; @@ -48,15 +49,17 @@ export const renderHtml = ({ const format: ArticleFormat = decideFormat(article.format); + const renderingContext: RenderingContextType = { target: 'Web' }; + const { html, extractedCss } = renderToStringWithEmotion( - + - , + , ); // We want to only insert script tags for the elements or main media elements on this page view diff --git a/dotcom-rendering/src/types/renderingContext.ts b/dotcom-rendering/src/types/renderingContext.ts new file mode 100644 index 00000000000..18ca7501bfd --- /dev/null +++ b/dotcom-rendering/src/types/renderingContext.ts @@ -0,0 +1,10 @@ +import type { RenderingTarget } from './renderingTarget'; + +/** + * Context for global values (generic) + * + * This should not contain any properties which are likely to change between re-renders + */ +export interface RenderingContextType { + target: RenderingTarget; +} From 91cfa08bccea0a69e7045c320b5622a9037a4be9 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Fri, 1 Sep 2023 11:57:50 +0100 Subject: [PATCH 03/20] ensure rendering target is reflected in layout stories context --- dotcom-rendering/scripts/gen-stories/get-stories.js | 1 + dotcom-rendering/stories/generated/Layout.stories.tsx | 3 +++ 2 files changed, 4 insertions(+) diff --git a/dotcom-rendering/scripts/gen-stories/get-stories.js b/dotcom-rendering/scripts/gen-stories/get-stories.js index 45c246776d8..7ab83894da9 100644 --- a/dotcom-rendering/scripts/gen-stories/get-stories.js +++ b/dotcom-rendering/scripts/gen-stories/get-stories.js @@ -119,6 +119,7 @@ export const ${storyVariableName} = () => { ); }; ${storyVariableName}.storyName = '${renderingTarget}: Display: ${displayName}, Design: ${designName}, Theme: ${theme}'; +${storyVariableName}.args = { renderingContext: { target: '${renderingTarget}' } }; `; }; diff --git a/dotcom-rendering/stories/generated/Layout.stories.tsx b/dotcom-rendering/stories/generated/Layout.stories.tsx index 89016c39757..41650a007b1 100644 --- a/dotcom-rendering/stories/generated/Layout.stories.tsx +++ b/dotcom-rendering/stories/generated/Layout.stories.tsx @@ -28,6 +28,7 @@ export const WebStandardStandardNewsPillar = () => { ); }; WebStandardStandardNewsPillar.storyName = 'Web: Display: Standard, Design: Standard, Theme: NewsPillar'; +WebStandardStandardNewsPillar.args = { renderingContext: { target: 'Web' } }; export const AppsStandardStandardNewsPillar = () => { return ( @@ -40,6 +41,7 @@ export const AppsStandardStandardNewsPillar = () => { ); }; AppsStandardStandardNewsPillar.storyName = 'Apps: Display: Standard, Design: Standard, Theme: NewsPillar'; +AppsStandardStandardNewsPillar.args = { renderingContext: { target: 'Apps' } }; export const WebShowcasePictureOpinionPillar = () => { return ( @@ -52,3 +54,4 @@ export const WebShowcasePictureOpinionPillar = () => { ); }; WebShowcasePictureOpinionPillar.storyName = 'Web: Display: Showcase, Design: Picture, Theme: OpinionPillar'; +WebShowcasePictureOpinionPillar.args = { renderingContext: { target: 'Web' } }; From fbe778c1a0b7e97c0c73f3c7e5ee90b89224ac60 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Mon, 4 Sep 2023 16:13:43 +0100 Subject: [PATCH 04/20] docs: add new react context proposal adr and rename adr sub dirs --- .../000-technical-review-records.md | 0 .../001-modi-poc.md | 0 .../002-ipsos-mori.md | 0 .../003-gracenote.md | 0 .../004-urlbox.md | 0 .../dotenv.md | 0 .../multiple-sites.md | 0 .../signing-image-urls.md | 0 ...percy-for-e2e-visual-regression-testing.md | 0 .../proposed-adrs/react-context-api.md | 22 +++++++++++++++++++ .../rendering-type.md | 0 11 files changed, 22 insertions(+) rename dotcom-rendering/docs/architecture/{3rd party technical review => 3rd-party-technical-review}/000-technical-review-records.md (100%) rename dotcom-rendering/docs/architecture/{3rd party technical review => 3rd-party-technical-review}/001-modi-poc.md (100%) rename dotcom-rendering/docs/architecture/{3rd party technical review => 3rd-party-technical-review}/002-ipsos-mori.md (100%) rename dotcom-rendering/docs/architecture/{3rd party technical review => 3rd-party-technical-review}/003-gracenote.md (100%) rename dotcom-rendering/docs/architecture/{3rd party technical review => 3rd-party-technical-review}/004-urlbox.md (100%) rename dotcom-rendering/docs/architecture/{historic adrs => historic-adrs}/dotenv.md (100%) rename dotcom-rendering/docs/architecture/{historic adrs => historic-adrs}/multiple-sites.md (100%) rename dotcom-rendering/docs/architecture/{historic adrs => historic-adrs}/signing-image-urls.md (100%) rename dotcom-rendering/docs/architecture/{historic adrs => historic-adrs}/use-percy-for-e2e-visual-regression-testing.md (100%) create mode 100644 dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md rename dotcom-rendering/docs/architecture/{proposed adrs => proposed-adrs}/rendering-type.md (100%) diff --git a/dotcom-rendering/docs/architecture/3rd party technical review/000-technical-review-records.md b/dotcom-rendering/docs/architecture/3rd-party-technical-review/000-technical-review-records.md similarity index 100% rename from dotcom-rendering/docs/architecture/3rd party technical review/000-technical-review-records.md rename to dotcom-rendering/docs/architecture/3rd-party-technical-review/000-technical-review-records.md diff --git a/dotcom-rendering/docs/architecture/3rd party technical review/001-modi-poc.md b/dotcom-rendering/docs/architecture/3rd-party-technical-review/001-modi-poc.md similarity index 100% rename from dotcom-rendering/docs/architecture/3rd party technical review/001-modi-poc.md rename to dotcom-rendering/docs/architecture/3rd-party-technical-review/001-modi-poc.md diff --git a/dotcom-rendering/docs/architecture/3rd party technical review/002-ipsos-mori.md b/dotcom-rendering/docs/architecture/3rd-party-technical-review/002-ipsos-mori.md similarity index 100% rename from dotcom-rendering/docs/architecture/3rd party technical review/002-ipsos-mori.md rename to dotcom-rendering/docs/architecture/3rd-party-technical-review/002-ipsos-mori.md diff --git a/dotcom-rendering/docs/architecture/3rd party technical review/003-gracenote.md b/dotcom-rendering/docs/architecture/3rd-party-technical-review/003-gracenote.md similarity index 100% rename from dotcom-rendering/docs/architecture/3rd party technical review/003-gracenote.md rename to dotcom-rendering/docs/architecture/3rd-party-technical-review/003-gracenote.md diff --git a/dotcom-rendering/docs/architecture/3rd party technical review/004-urlbox.md b/dotcom-rendering/docs/architecture/3rd-party-technical-review/004-urlbox.md similarity index 100% rename from dotcom-rendering/docs/architecture/3rd party technical review/004-urlbox.md rename to dotcom-rendering/docs/architecture/3rd-party-technical-review/004-urlbox.md diff --git a/dotcom-rendering/docs/architecture/historic adrs/dotenv.md b/dotcom-rendering/docs/architecture/historic-adrs/dotenv.md similarity index 100% rename from dotcom-rendering/docs/architecture/historic adrs/dotenv.md rename to dotcom-rendering/docs/architecture/historic-adrs/dotenv.md diff --git a/dotcom-rendering/docs/architecture/historic adrs/multiple-sites.md b/dotcom-rendering/docs/architecture/historic-adrs/multiple-sites.md similarity index 100% rename from dotcom-rendering/docs/architecture/historic adrs/multiple-sites.md rename to dotcom-rendering/docs/architecture/historic-adrs/multiple-sites.md diff --git a/dotcom-rendering/docs/architecture/historic adrs/signing-image-urls.md b/dotcom-rendering/docs/architecture/historic-adrs/signing-image-urls.md similarity index 100% rename from dotcom-rendering/docs/architecture/historic adrs/signing-image-urls.md rename to dotcom-rendering/docs/architecture/historic-adrs/signing-image-urls.md diff --git a/dotcom-rendering/docs/architecture/historic adrs/use-percy-for-e2e-visual-regression-testing.md b/dotcom-rendering/docs/architecture/historic-adrs/use-percy-for-e2e-visual-regression-testing.md similarity index 100% rename from dotcom-rendering/docs/architecture/historic adrs/use-percy-for-e2e-visual-regression-testing.md rename to dotcom-rendering/docs/architecture/historic-adrs/use-percy-for-e2e-visual-regression-testing.md diff --git a/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md b/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md new file mode 100644 index 00000000000..48baa45fde2 --- /dev/null +++ b/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md @@ -0,0 +1,22 @@ +# React Context API + +## Context + +[A decision](dotcom-rendering/docs/architecture/016-react-context-api.md) was made in 2019 to avoid use of the React context API, preferring prop-drilling and avoiding global state. + +[This decision was revisited in 2023](https://github.com/guardian/dotcom-rendering/discussions/8696) due to [the introduction of rendering target as a prop](dotcom-rendering/docs/architecture/proposed-adrs/rendering-type.md) which represents a global state, resulting in very heavy prop-drilling throughout the app. This began to complicate pull requests due to so many unrelated changes appearing in the diff since the props needed to be provided at every layer of the app, as well as tightly coupling components unnecessarily. + +An RFC was put together [here](https://github.com/guardian/dotcom-rendering/pull/8704) to trial using the React context API for this specific type of global state, which doesn't change throughout the component lifecycle. + +## Decision + +The decision to allow use of context more generally rests on the following _"lines in the sand"_: + +- Context should be considered **global** for rendered components in dotcom-rendering +- Context should **only be used for JSX components** (ie. not for JSON responses or non-JSX helper code) +- Context should **not be used for values that change between re-renders**, since this adds unnecessary complexity and there are alternative solutions for these cases +- Context should **only be used for server-side rendered (SSR) components** and therefore should **not be used inside islands** + +## Status + +Proposed diff --git a/dotcom-rendering/docs/architecture/proposed adrs/rendering-type.md b/dotcom-rendering/docs/architecture/proposed-adrs/rendering-type.md similarity index 100% rename from dotcom-rendering/docs/architecture/proposed adrs/rendering-type.md rename to dotcom-rendering/docs/architecture/proposed-adrs/rendering-type.md From 8414c5ff6875086dab8e3c924d23eacdedc6da34 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Mon, 4 Sep 2023 17:47:06 +0100 Subject: [PATCH 05/20] docs: add storybook docs to help explain context and/or decorators --- dotcom-rendering/docs/development/storybook.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 dotcom-rendering/docs/development/storybook.md diff --git a/dotcom-rendering/docs/development/storybook.md b/dotcom-rendering/docs/development/storybook.md new file mode 100644 index 00000000000..47316d6b310 --- /dev/null +++ b/dotcom-rendering/docs/development/storybook.md @@ -0,0 +1,12 @@ +# Storybook + +We use Storybook to visualise our components in an isolated environment, where we can tweak the conditions to our hearts content. +We use Chromatic for visual regression testing of Storybook components. + +Here is a non-exhaustive list of some useful tips for our Storybook implementation: + +## Rendering context + +We use context for static, global state in the dotcom-rendering app, so every story is wrapped in a context provider component. In the real world, our top component includes this context provider. + +In Storybook is largely invisible as it's hidden within the [configuration](dotcom-rendering/.storybook). There's a decorator configured to wrap around stories and log the context output to the console, for easier debugging. From d92d8da386d763e229d06fa8b7696e220ce2dea1 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Mon, 4 Sep 2023 17:48:31 +0100 Subject: [PATCH 06/20] add context hook to improve ease of use of context object --- .../src/components/ArticleMeta.stories.tsx | 1 + .../src/components/BylineLink.tsx | 7 ++-- .../src/components/RenderingContext.tsx | 40 ++++++++++++++++++- dotcom-rendering/src/lib/ArticleRenderer.tsx | 7 ++-- .../src/types/renderingContext.ts | 9 +++++ 5 files changed, 54 insertions(+), 10 deletions(-) diff --git a/dotcom-rendering/src/components/ArticleMeta.stories.tsx b/dotcom-rendering/src/components/ArticleMeta.stories.tsx index f484e63228b..83085204b1c 100644 --- a/dotcom-rendering/src/components/ArticleMeta.stories.tsx +++ b/dotcom-rendering/src/components/ArticleMeta.stories.tsx @@ -114,6 +114,7 @@ export const ArticleAppsStory = () => { ); }; +/** @see /dotcom-rendering/docs/development/storybook.md */ ArticleAppsStory.args = { renderingContext: { target: 'Apps' } }; export const BrandingStory = () => { diff --git a/dotcom-rendering/src/components/BylineLink.tsx b/dotcom-rendering/src/components/BylineLink.tsx index 204591c5325..a18357bd8d9 100644 --- a/dotcom-rendering/src/components/BylineLink.tsx +++ b/dotcom-rendering/src/components/BylineLink.tsx @@ -1,5 +1,4 @@ import { ArticleDesign } from '@guardian/libs'; -import { useContext } from 'react'; import { getBylineComponentsFromTokens, getSoleContributor, @@ -8,7 +7,7 @@ import { import type { TagType } from '../types/tag'; import { FollowWrapper } from './FollowWrapper.importable'; import { Island } from './Island'; -import { RenderingContext } from './RenderingContext'; +import { useRenderingTarget } from './RenderingContext'; type Props = { byline: string; @@ -132,12 +131,12 @@ export const BylineLink = ({ byline, tags, format }: Props) => { ); }); - const { target } = useContext(RenderingContext); + const { isApps } = useRenderingTarget(); return ( <> {renderedTokens} - {target === 'Apps' && soleContributor !== undefined ? ( + {isApps && soleContributor !== undefined ? ( (defaultContext); + +/** + * useContext hook for rendering target details + * + * @example + * const { isWeb } = useRenderingTarget(); + * + * if (isWeb) { + * ... + * } else { + * ... + * } + * + */ +export const useRenderingTarget = (): RenderingTargetContextType => { + const context = useContext(RenderingContext); + + if (context === undefined) { + throw Error( + 'useRenderingTarget must be used within the RenderingContext provider', + ); + } + + const { target } = context; + + const renderingTarget = { + target, + isWeb: target === 'Web', + isApps: target === 'Apps', + }; + + return renderingTarget; +}; diff --git a/dotcom-rendering/src/lib/ArticleRenderer.tsx b/dotcom-rendering/src/lib/ArticleRenderer.tsx index 95c268f88d6..8ad914d599f 100644 --- a/dotcom-rendering/src/lib/ArticleRenderer.tsx +++ b/dotcom-rendering/src/lib/ArticleRenderer.tsx @@ -1,9 +1,8 @@ import { css } from '@emotion/react'; import type { ArticleFormat } from '@guardian/libs'; import { ArticleDesign } from '@guardian/libs'; -import { useContext } from 'react'; import { adContainerStyles } from '../components/AdSlot'; -import { RenderingContext } from '../components/RenderingContext'; +import { useRenderingTarget } from '../components/RenderingContext'; import { interactiveLegacyClasses } from '../layouts/lib/interactiveLegacyStyling'; import type { ServerSideTests, Switches } from '../types/config'; import type { FEElement } from '../types/content'; @@ -85,7 +84,7 @@ export const ArticleRenderer = ({ ); }); - const { target } = useContext(RenderingContext); + const { isApps } = useRenderingTarget(); // const cleanedElements = elements.map(element => // 'html' in element ? { ...element, html: clean(element.html) } : element, @@ -106,7 +105,7 @@ export const ArticleRenderer = ({ ].join(' ')} css={[adStylesDynamic, commercialPosition]} > - {target === 'Apps' + {isApps ? renderedElements : /* Insert the placeholder for the sign in gate on the 2nd article element */ withSignInGateSlot({ diff --git a/dotcom-rendering/src/types/renderingContext.ts b/dotcom-rendering/src/types/renderingContext.ts index 18ca7501bfd..2003622481c 100644 --- a/dotcom-rendering/src/types/renderingContext.ts +++ b/dotcom-rendering/src/types/renderingContext.ts @@ -8,3 +8,12 @@ import type { RenderingTarget } from './renderingTarget'; export interface RenderingContextType { target: RenderingTarget; } + +/** + * Context for rendering target, specifically + */ +export interface RenderingTargetContextType { + target: RenderingTarget; + isApps: boolean; + isWeb: boolean; +} From 174def7bb7f454151cd7f4c3e7ca644f7abd797a Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 6 Sep 2023 10:13:12 +0100 Subject: [PATCH 07/20] ensure useContext hook cannot be used outside of relevant provider --- .../docs/development/storybook.md | 4 +-- .../src/components/BylineLink.tsx | 7 ++-- .../src/components/RenderingContext.tsx | 33 +++++++++---------- dotcom-rendering/src/lib/ArticleRenderer.tsx | 4 +-- .../src/types/renderingContext.ts | 8 ++--- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/dotcom-rendering/docs/development/storybook.md b/dotcom-rendering/docs/development/storybook.md index 47316d6b310..88041adccf1 100644 --- a/dotcom-rendering/docs/development/storybook.md +++ b/dotcom-rendering/docs/development/storybook.md @@ -1,10 +1,8 @@ # Storybook -We use Storybook to visualise our components in an isolated environment, where we can tweak the conditions to our hearts content. +We use Storybook to visualise our components in an isolated environment, where we can tweak the conditions as we want. We use Chromatic for visual regression testing of Storybook components. -Here is a non-exhaustive list of some useful tips for our Storybook implementation: - ## Rendering context We use context for static, global state in the dotcom-rendering app, so every story is wrapped in a context provider component. In the real world, our top component includes this context provider. diff --git a/dotcom-rendering/src/components/BylineLink.tsx b/dotcom-rendering/src/components/BylineLink.tsx index a18357bd8d9..5055b760331 100644 --- a/dotcom-rendering/src/components/BylineLink.tsx +++ b/dotcom-rendering/src/components/BylineLink.tsx @@ -7,7 +7,7 @@ import { import type { TagType } from '../types/tag'; import { FollowWrapper } from './FollowWrapper.importable'; import { Island } from './Island'; -import { useRenderingTarget } from './RenderingContext'; +import { useRenderingContext } from './RenderingContext'; type Props = { byline: string; @@ -131,12 +131,13 @@ export const BylineLink = ({ byline, tags, format }: Props) => { ); }); - const { isApps } = useRenderingTarget(); + const renderingContext = useRenderingContext(); + console.log('=====> RENDERING CONTEXT', renderingContext); return ( <> {renderedTokens} - {isApps && soleContributor !== undefined ? ( + {renderingContext.isApps && soleContributor !== undefined ? ( (defaultContext); +const RenderingContext = createContext( + undefined, +); /** - * useContext hook for rendering target details + * useContext hook for safely fetching the rendering context value + * Ensures that it is used within a relevant Context.Provider * * @example - * const { isWeb } = useRenderingTarget(); + * const { isWeb } = useRenderingContext(); * * if (isWeb) { * ... * } else { * ... * } - * */ -export const useRenderingTarget = (): RenderingTargetContextType => { +export const useRenderingContext = (): EnhancedRenderingContextType => { const context = useContext(RenderingContext); if (context === undefined) { throw Error( - 'useRenderingTarget must be used within the RenderingContext provider', + 'useRenderingContext must be used within the RenderingContext provider', ); } const { target } = context; - const renderingTarget = { + return { target, isWeb: target === 'Web', isApps: target === 'Apps', }; - - return renderingTarget; }; diff --git a/dotcom-rendering/src/lib/ArticleRenderer.tsx b/dotcom-rendering/src/lib/ArticleRenderer.tsx index 8ad914d599f..41d9c1828e8 100644 --- a/dotcom-rendering/src/lib/ArticleRenderer.tsx +++ b/dotcom-rendering/src/lib/ArticleRenderer.tsx @@ -2,7 +2,7 @@ import { css } from '@emotion/react'; import type { ArticleFormat } from '@guardian/libs'; import { ArticleDesign } from '@guardian/libs'; import { adContainerStyles } from '../components/AdSlot'; -import { useRenderingTarget } from '../components/RenderingContext'; +import { useRenderingContext } from '../components/RenderingContext'; import { interactiveLegacyClasses } from '../layouts/lib/interactiveLegacyStyling'; import type { ServerSideTests, Switches } from '../types/config'; import type { FEElement } from '../types/content'; @@ -84,7 +84,7 @@ export const ArticleRenderer = ({ ); }); - const { isApps } = useRenderingTarget(); + const { isApps } = useRenderingContext(); // const cleanedElements = elements.map(element => // 'html' in element ? { ...element, html: clean(element.html) } : element, diff --git a/dotcom-rendering/src/types/renderingContext.ts b/dotcom-rendering/src/types/renderingContext.ts index 2003622481c..de191c51d78 100644 --- a/dotcom-rendering/src/types/renderingContext.ts +++ b/dotcom-rendering/src/types/renderingContext.ts @@ -3,17 +3,17 @@ import type { RenderingTarget } from './renderingTarget'; /** * Context for global values (generic) * - * This should not contain any properties which are likely to change between re-renders + * Do not add properties which are likely to change between re-renders + * @see /dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md */ export interface RenderingContextType { target: RenderingTarget; } /** - * Context for rendering target, specifically + * Enhanced context based on provided values, making assertions easier */ -export interface RenderingTargetContextType { - target: RenderingTarget; +export interface EnhancedRenderingContextType extends RenderingContextType { isApps: boolean; isWeb: boolean; } From 9d41ac0dfce371cb9258c712d8b1ae9a3f2d08b9 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 6 Sep 2023 10:20:32 +0100 Subject: [PATCH 08/20] move provider down tree and prevent direct context use --- dotcom-rendering/.eslintrc.js | 2 + .../src/components/ArticlePage.tsx | 195 +++++++++--------- .../src/components/RenderingContext.tsx | 19 +- .../src/server/render.article.apps.tsx | 16 +- .../src/server/render.article.web.tsx | 18 +- 5 files changed, 134 insertions(+), 116 deletions(-) diff --git a/dotcom-rendering/.eslintrc.js b/dotcom-rendering/.eslintrc.js index fb1d8115f7c..36a83993764 100644 --- a/dotcom-rendering/.eslintrc.js +++ b/dotcom-rendering/.eslintrc.js @@ -107,6 +107,8 @@ module.exports = { ignoreNonDOM: true, }, ], + // We want to be careful with context and certainly avoid unnecessary re-renders + 'react/jsx-no-constructed-context-values': 'error', '@typescript-eslint/switch-exhaustiveness-check': 'error', 'array-callback-return': 'error', diff --git a/dotcom-rendering/src/components/ArticlePage.tsx b/dotcom-rendering/src/components/ArticlePage.tsx index b5538b552e9..20470c15a23 100644 --- a/dotcom-rendering/src/components/ArticlePage.tsx +++ b/dotcom-rendering/src/components/ArticlePage.tsx @@ -17,6 +17,7 @@ import { LightboxJavascript } from './LightboxJavascript.importable'; import { LightboxLayout } from './LightboxLayout'; import { Metrics } from './Metrics.importable'; import { ReaderRevenueDev } from './ReaderRevenueDev.importable'; +import { RenderingContextProvider } from './RenderingContext'; import { SendTargetingParams } from './SendTargetingParams.importable'; import { SetABTests } from './SetABTests.importable'; import { SetAdTargeting } from './SetAdTargeting.importable'; @@ -53,107 +54,117 @@ export const ArticlePage = (props: WebProps | AppProps) => { adUnit: article.config.adUnit, }); + const renderingContext = { target: renderingTarget }; + return ( - - - - {article.config.switches.lightbox && article.imagesForLightbox && ( - <> - + + + + {article.config.switches.lightbox && + article.imagesForLightbox && ( + <> + + + + + + + + + )} + + + + {(format.design === ArticleDesign.LiveBlog || + format.design === ArticleDesign.DeadBlog) && ( + + )} + {renderingTarget === 'Web' && ( + <> + + + + + + + + + + + + + + + + + + )} + {renderingTarget === 'Web' ? ( - - - - - - - )} - - - - {(format.design === ArticleDesign.LiveBlog || - format.design === ArticleDesign.DeadBlog) && ( - - )} - {renderingTarget === 'Web' && ( - <> - - - - - - - - - - - - + + ) : ( - - - )} - {renderingTarget === 'Web' ? ( - - - - ) : ( - - - - )} - {renderingTarget === 'Apps' ? ( - - ) : ( - - )} + ) : ( + + )} + ); }; diff --git a/dotcom-rendering/src/components/RenderingContext.tsx b/dotcom-rendering/src/components/RenderingContext.tsx index bad5f19eb15..4946524a9fe 100644 --- a/dotcom-rendering/src/components/RenderingContext.tsx +++ b/dotcom-rendering/src/components/RenderingContext.tsx @@ -1,4 +1,4 @@ -import { createContext, useContext } from 'react'; +import { createContext, useContext, useMemo } from 'react'; import type { EnhancedRenderingContextType, RenderingContextType, @@ -18,6 +18,23 @@ const RenderingContext = createContext( undefined, ); +export const RenderingContextProvider = ({ + value, + children, +}: { + value: RenderingContextType; + children: React.ReactNode; +}) => { + // useMemo aims to reduce unnecessary re-renders for context + const memoisedValue = useMemo(() => value, [value]); + + return ( + + {children} + + ); +}; + /** * useContext hook for safely fetching the rendering context value * Ensures that it is used within a relevant Context.Provider diff --git a/dotcom-rendering/src/server/render.article.apps.tsx b/dotcom-rendering/src/server/render.article.apps.tsx index 44c37db7c75..30f8faa4698 100644 --- a/dotcom-rendering/src/server/render.article.apps.tsx +++ b/dotcom-rendering/src/server/render.article.apps.tsx @@ -1,12 +1,10 @@ import { isString } from '@guardian/libs'; import { ArticlePage } from '../components/ArticlePage'; -import { RenderingContext } from '../components/RenderingContext'; import { generateScriptTags, getPathFromManifest } from '../lib/assets'; import { decideFormat } from '../lib/decideFormat'; import { renderToStringWithEmotion } from '../lib/emotion'; import { createGuardian } from '../model/guardian'; import type { FEArticleType } from '../types/frontend'; -import type { RenderingContextType } from '../types/renderingContext'; import { htmlPageTemplate } from './htmlPageTemplate'; export const renderArticle = ( @@ -17,16 +15,12 @@ export const renderArticle = ( } => { const format: ArticleFormat = decideFormat(article.format); - const renderingContext: RenderingContextType = { target: 'Apps' }; - const { html, extractedCss } = renderToStringWithEmotion( - - - , + , ); const clientScripts = [getPathFromManifest('apps', 'index.js')]; diff --git a/dotcom-rendering/src/server/render.article.web.tsx b/dotcom-rendering/src/server/render.article.web.tsx index 856c02c3b5f..4ab84f7d7f2 100644 --- a/dotcom-rendering/src/server/render.article.web.tsx +++ b/dotcom-rendering/src/server/render.article.web.tsx @@ -2,7 +2,6 @@ import { ArticleDesign, isString, Pillar } from '@guardian/libs'; import { ArticlePage } from '../components/ArticlePage'; import { isAmpSupported } from '../components/Elements.amp'; import { KeyEventsContainer } from '../components/KeyEventsContainer'; -import { RenderingContext } from '../components/RenderingContext'; import { ASSET_ORIGIN, generateScriptTags, @@ -21,7 +20,6 @@ import { extractNAV } from '../model/extract-nav'; import { createGuardian as createWindowGuardian } from '../model/guardian'; import type { FEElement } from '../types/content'; import type { FEArticleType, FEBlocksRequest } from '../types/frontend'; -import type { RenderingContextType } from '../types/renderingContext'; import type { TagType } from '../types/tag'; import { htmlPageTemplate } from './htmlPageTemplate'; @@ -49,17 +47,13 @@ export const renderHtml = ({ const format: ArticleFormat = decideFormat(article.format); - const renderingContext: RenderingContextType = { target: 'Web' }; - const { html, extractedCss } = renderToStringWithEmotion( - - - , + , ); // We want to only insert script tags for the elements or main media elements on this page view From 6ac0184f1c508a95b9bd90756150aaa39462350b Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 6 Sep 2023 11:25:50 +0100 Subject: [PATCH 09/20] remove console.log --- dotcom-rendering/src/components/BylineLink.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dotcom-rendering/src/components/BylineLink.tsx b/dotcom-rendering/src/components/BylineLink.tsx index 5055b760331..624c72fecbe 100644 --- a/dotcom-rendering/src/components/BylineLink.tsx +++ b/dotcom-rendering/src/components/BylineLink.tsx @@ -131,13 +131,12 @@ export const BylineLink = ({ byline, tags, format }: Props) => { ); }); - const renderingContext = useRenderingContext(); - console.log('=====> RENDERING CONTEXT', renderingContext); + const { isApps } = useRenderingContext(); return ( <> {renderedTokens} - {renderingContext.isApps && soleContributor !== undefined ? ( + {isApps && soleContributor !== undefined ? ( Date: Wed, 6 Sep 2023 16:09:47 +0100 Subject: [PATCH 10/20] rename context to config, simplify implementation, add islands support --- .../decorators/configContextDecorator.js | 16 +++++ .../decorators/renderingContextDecorator.js | 19 ------ dotcom-rendering/.storybook/preview.js | 4 +- dotcom-rendering/index.d.ts | 5 ++ .../scripts/gen-stories/get-stories.js | 2 +- dotcom-rendering/src/client/discussion.ts | 6 +- .../src/client/islands/doHydration.tsx | 20 ++++-- .../client/islands/doStorybookHydration.js | 17 +++-- .../src/client/islands/getConfig.ts | 28 ++++++++ .../src/client/islands/initHydration.ts | 38 ++++++++--- .../src/components/ArticleMeta.stories.tsx | 2 +- .../src/components/ArticlePage.tsx | 15 +++-- .../src/components/BylineLink.tsx | 6 +- .../src/components/ConfigContext.tsx | 41 ++++++++++++ dotcom-rendering/src/components/Island.tsx | 30 +++++---- .../src/components/RenderingContext.tsx | 67 ------------------- dotcom-rendering/src/lib/ArticleRenderer.tsx | 6 +- dotcom-rendering/src/types/configContext.ts | 11 +++ .../src/types/renderingContext.ts | 19 ------ .../stories/generated/Layout.stories.tsx | 6 +- 20 files changed, 200 insertions(+), 158 deletions(-) create mode 100644 dotcom-rendering/.storybook/decorators/configContextDecorator.js delete mode 100644 dotcom-rendering/.storybook/decorators/renderingContextDecorator.js create mode 100644 dotcom-rendering/src/client/islands/getConfig.ts create mode 100644 dotcom-rendering/src/components/ConfigContext.tsx delete mode 100644 dotcom-rendering/src/components/RenderingContext.tsx create mode 100644 dotcom-rendering/src/types/configContext.ts delete mode 100644 dotcom-rendering/src/types/renderingContext.ts diff --git a/dotcom-rendering/.storybook/decorators/configContextDecorator.js b/dotcom-rendering/.storybook/decorators/configContextDecorator.js new file mode 100644 index 00000000000..f04f24d662b --- /dev/null +++ b/dotcom-rendering/.storybook/decorators/configContextDecorator.js @@ -0,0 +1,16 @@ +import { ConfigProvider } from '../../src/components/ConfigContext'; + +const defaultConfig = { renderingTarget: 'Web' }; + +export const ConfigContextDecorator = (Story, { args: { config } }) => { + const context = { ...defaultConfig, ...config }; + + // For easy debugging + console.log('Storybook application config: \n', context); + + return ( + + + + ); +}; diff --git a/dotcom-rendering/.storybook/decorators/renderingContextDecorator.js b/dotcom-rendering/.storybook/decorators/renderingContextDecorator.js deleted file mode 100644 index e95426c7cee..00000000000 --- a/dotcom-rendering/.storybook/decorators/renderingContextDecorator.js +++ /dev/null @@ -1,19 +0,0 @@ -import { RenderingContext } from '../../src/components/RenderingContext'; - -const defaultContext = { target: 'Web' }; - -export const RenderingContextDecorator = ( - Story, - { args: { renderingContext } }, -) => { - const context = { ...defaultContext, ...renderingContext }; - - // For easy debugging - console.log('Storybook rendering context: \n', context); - - return ( - - - - ); -}; diff --git a/dotcom-rendering/.storybook/preview.js b/dotcom-rendering/.storybook/preview.js index 6d6c28b5c11..8d78fa8b7bc 100644 --- a/dotcom-rendering/.storybook/preview.js +++ b/dotcom-rendering/.storybook/preview.js @@ -12,7 +12,7 @@ import { Lazy } from '../src/components/Lazy'; import { Picture } from '../src/components/Picture'; import { mockRESTCalls } from '../src/lib/mockRESTCalls'; import { setABTests } from '../src/lib/useAB'; -import { RenderingContextDecorator } from './decorators/renderingContextDecorator'; +import { ConfigContextDecorator } from './decorators/configContextDecorator'; // Prevent components being lazy rendered when we're taking Chromatic snapshots Lazy.disabled = isChromatic(); @@ -140,7 +140,7 @@ const guardianViewports = { /** @type {import('@storybook/react').Preview} */ export default { decorators: [ - RenderingContextDecorator, + ConfigContextDecorator, (Story) => { storage.local.clear(); return Story(); diff --git a/dotcom-rendering/index.d.ts b/dotcom-rendering/index.d.ts index 73d6c55a20f..b10e2982ccd 100644 --- a/dotcom-rendering/index.d.ts +++ b/dotcom-rendering/index.d.ts @@ -521,6 +521,11 @@ declare namespace JSX { clientOnly?: boolean; props: any; children: React.ReactNode; + /** + * This should be a stringified JSON of `ConfigContext` + * @see /dotcom-rendering/src/types/configContext.ts + */ + config: string; }; } diff --git a/dotcom-rendering/scripts/gen-stories/get-stories.js b/dotcom-rendering/scripts/gen-stories/get-stories.js index 7ab83894da9..29a6b9768bb 100644 --- a/dotcom-rendering/scripts/gen-stories/get-stories.js +++ b/dotcom-rendering/scripts/gen-stories/get-stories.js @@ -119,7 +119,7 @@ export const ${storyVariableName} = () => { ); }; ${storyVariableName}.storyName = '${renderingTarget}: Display: ${displayName}, Design: ${designName}, Theme: ${theme}'; -${storyVariableName}.args = { renderingContext: { target: '${renderingTarget}' } }; +${storyVariableName}.args = { config: { renderingTarget: '${renderingTarget}' } }; `; }; diff --git a/dotcom-rendering/src/client/discussion.ts b/dotcom-rendering/src/client/discussion.ts index e71301193bf..76c1a06201f 100644 --- a/dotcom-rendering/src/client/discussion.ts +++ b/dotcom-rendering/src/client/discussion.ts @@ -1,5 +1,6 @@ import { doHydration } from './islands/doHydration'; import { getEmotionCache } from './islands/emotion'; +import { getConfig } from './islands/getConfig'; import { getProps } from './islands/getProps'; const forceHydration = async (): Promise => { @@ -12,14 +13,15 @@ const forceHydration = async (): Promise => { ); if (!guElement) return; - // Read the props from where they have been serialised in the dom using an Island + // Read the props and config from where they have been serialised in the dom using an Island const props = getProps(guElement); + const config = getConfig(guElement); // Now that we have the props as an object, tell Discussion we want it to expand itself props.expanded = true; // Force hydration - await doHydration(name, props, guElement, getEmotionCache()); + await doHydration(name, props, guElement, getEmotionCache(), config); } catch (err) { // Do nothing } diff --git a/dotcom-rendering/src/client/islands/doHydration.tsx b/dotcom-rendering/src/client/islands/doHydration.tsx index 88abe23a60c..80f4e40b172 100644 --- a/dotcom-rendering/src/client/islands/doHydration.tsx +++ b/dotcom-rendering/src/client/islands/doHydration.tsx @@ -4,6 +4,8 @@ import { CacheProvider } from '@emotion/react'; import { log, startPerformanceMeasure } from '@guardian/libs'; import { createElement } from 'react'; import { createRoot, hydrateRoot } from 'react-dom/client'; +import { ConfigProvider } from '../../components/ConfigContext'; +import type { ApplicationConfig } from '../../types/configContext'; declare global { interface DOMStringMap { @@ -26,12 +28,14 @@ declare global { * @param data The deserialised props we want to use for hydration * @param element The location on the DOM where the component to hydrate exists * @param emotionCache An instance of an emotion cache to use for the island + * @param config Application configuration to be passed to the config context for the hydrated component */ export const doHydration = async ( name: string, data: { [key: string]: unknown } | null, element: HTMLElement, emotionCache: EmotionCache, + config: ApplicationConfig, ): Promise => { // If this function has already been run for an element then don't try to // run it a second time @@ -58,16 +62,20 @@ export const doHydration = async ( element.querySelector('[data-name="placeholder"]')?.remove(); const root = createRoot(element); root.render( - - {createElement(module[name], data)} - , + + + {createElement(module[name], data)} + + , ); } else { hydrateRoot( element, - - {createElement(module[name], data)} - , + + + {createElement(module[name], data)} + + , ); } diff --git a/dotcom-rendering/src/client/islands/doStorybookHydration.js b/dotcom-rendering/src/client/islands/doStorybookHydration.js index de3b089d384..f3431fed6ce 100644 --- a/dotcom-rendering/src/client/islands/doStorybookHydration.js +++ b/dotcom-rendering/src/client/islands/doStorybookHydration.js @@ -1,5 +1,7 @@ import { createElement } from 'react'; import { createRoot } from 'react-dom/client'; +import { ConfigProvider } from '../../components/ConfigContext'; +import { getConfig } from './getConfig'; import { getName } from './getName'; import { getProps } from './getProps'; @@ -16,13 +18,14 @@ import { getProps } from './getProps'; * snapshots */ export const doStorybookHydration = () => { - document.querySelectorAll('gu-island').forEach((element) => { + for (const element of document.querySelectorAll('gu-island')) { if (element instanceof HTMLElement) { const name = getName(element); const props = getProps(element); + const config = getConfig(element); - if (!name) return; - if (element.getAttribute('deferuntil') === 'hash') return; + if (!name) continue; + if (element.getAttribute('deferuntil') === 'hash') continue; import( /* webpackInclude: /\.importable\.tsx$/ */ @@ -34,7 +37,11 @@ export const doStorybookHydration = () => { .querySelector('[data-name="placeholder"]') ?.remove(); const root = createRoot(element); - root.render(createElement(module[name], props)); + root.render( + + {createElement(module[name], props)} + , + ); }) .catch((e) => // eslint-disable-next-line no-console -- We want to log here @@ -44,5 +51,5 @@ export const doStorybookHydration = () => { ), ); } - }); + } }; diff --git a/dotcom-rendering/src/client/islands/getConfig.ts b/dotcom-rendering/src/client/islands/getConfig.ts new file mode 100644 index 00000000000..52460ecc43f --- /dev/null +++ b/dotcom-rendering/src/client/islands/getConfig.ts @@ -0,0 +1,28 @@ +import type { ApplicationConfig } from '../../types/configContext'; + +/** + * getConfig takes the given html element and returns its config attribute + * + * We expect the element to always be a `gu-*` custom element + * + * @param marker : The html element that we want to read the config attribute from; + * @returns + */ +export const getConfig = (marker: HTMLElement): ApplicationConfig => { + const serialised = marker.getAttribute('config'); + + try { + if (!serialised) { + throw Error('Unable to fetch config attribute from marker element'); + } else { + return JSON.parse(serialised) as ApplicationConfig; + } + } catch (error: unknown) { + console.error( + `🚨 Error parsing config. Is this data serialisable? ${String( + serialised, + )} 🚨`, + ); + throw error; + } +}; diff --git a/dotcom-rendering/src/client/islands/initHydration.ts b/dotcom-rendering/src/client/islands/initHydration.ts index 5b73e2cc2a9..b8b2bdbec25 100644 --- a/dotcom-rendering/src/client/islands/initHydration.ts +++ b/dotcom-rendering/src/client/islands/initHydration.ts @@ -1,5 +1,6 @@ import type { EmotionCache } from '@emotion/cache'; import { doHydration } from './doHydration'; +import { getConfig } from './getConfig'; import { getName } from './getName'; import { getProps } from './getProps'; import { onInteraction } from './onInteraction'; @@ -36,6 +37,7 @@ export const initHydration = async ( ): Promise => { const name = getName(element); const props = getProps(element); + const config = getConfig(element); if (!name) return; @@ -43,7 +45,7 @@ export const initHydration = async ( switch (deferUntil) { case 'idle': { whenIdle(() => { - void doHydration(name, props, element, emotionCache); + void doHydration(name, props, element, emotionCache, config); }); return; } @@ -52,7 +54,13 @@ export const initHydration = async ( whenVisible( element, () => { - void doHydration(name, props, element, emotionCache); + void doHydration( + name, + props, + element, + emotionCache, + config, + ); }, { rootMargin }, ); @@ -60,17 +68,21 @@ export const initHydration = async ( } case 'interaction': { onInteraction(element, (targetElement) => { - void doHydration(name, props, element, emotionCache).then( - () => { - targetElement.dispatchEvent(new MouseEvent('click')); - }, - ); + void doHydration( + name, + props, + element, + emotionCache, + config, + ).then(() => { + targetElement.dispatchEvent(new MouseEvent('click')); + }); }); return; } case 'hash': { if (window.location.hash.includes(name) || hasLightboxHash(name)) { - void doHydration(name, props, element, emotionCache); + void doHydration(name, props, element, emotionCache, config); } else { // If we didn't find a matching hash on page load, set a // listener so that we check again each time the reader @@ -80,14 +92,20 @@ export const initHydration = async ( window.location.hash.includes(name) || hasLightboxHash(name) ) { - void doHydration(name, props, element, emotionCache); + void doHydration( + name, + props, + element, + emotionCache, + config, + ); } }); } return; } default: { - return doHydration(name, props, element, emotionCache); + return doHydration(name, props, element, emotionCache, config); } } }; diff --git a/dotcom-rendering/src/components/ArticleMeta.stories.tsx b/dotcom-rendering/src/components/ArticleMeta.stories.tsx index 83085204b1c..44f3b0a6002 100644 --- a/dotcom-rendering/src/components/ArticleMeta.stories.tsx +++ b/dotcom-rendering/src/components/ArticleMeta.stories.tsx @@ -115,7 +115,7 @@ export const ArticleAppsStory = () => { ); }; /** @see /dotcom-rendering/docs/development/storybook.md */ -ArticleAppsStory.args = { renderingContext: { target: 'Apps' } }; +ArticleAppsStory.args = { config: { renderingTarget: 'Apps' } }; export const BrandingStory = () => { return ( diff --git a/dotcom-rendering/src/components/ArticlePage.tsx b/dotcom-rendering/src/components/ArticlePage.tsx index 20470c15a23..24c79c5bc3c 100644 --- a/dotcom-rendering/src/components/ArticlePage.tsx +++ b/dotcom-rendering/src/components/ArticlePage.tsx @@ -10,6 +10,7 @@ import type { FEArticleType } from '../types/frontend'; import type { RenderingTarget } from '../types/renderingTarget'; import { AlreadyVisited } from './AlreadyVisited.importable'; import { BrazeMessaging } from './BrazeMessaging.importable'; +import { ConfigProvider } from './ConfigContext'; import { FocusStyles } from './FocusStyles.importable'; import { Island } from './Island'; import { LightboxHash } from './LightboxHash.importable'; @@ -17,7 +18,6 @@ import { LightboxJavascript } from './LightboxJavascript.importable'; import { LightboxLayout } from './LightboxLayout'; import { Metrics } from './Metrics.importable'; import { ReaderRevenueDev } from './ReaderRevenueDev.importable'; -import { RenderingContextProvider } from './RenderingContext'; import { SendTargetingParams } from './SendTargetingParams.importable'; import { SetABTests } from './SetABTests.importable'; import { SetAdTargeting } from './SetAdTargeting.importable'; @@ -40,7 +40,8 @@ interface AppProps extends BaseProps { /** * @description - * Article is a high level wrapper for article pages on Dotcom. Sets strict mode and some globals + * Article is a high level wrapper for article pages on Dotcom. + * Sets strict mode and some globals, as well as providing high-level context. * */ export const ArticlePage = (props: WebProps | AppProps) => { const { article, format, renderingTarget } = props; @@ -54,11 +55,15 @@ export const ArticlePage = (props: WebProps | AppProps) => { adUnit: article.config.adUnit, }); - const renderingContext = { target: renderingTarget }; + /** + * The static, immutable context value for the app + * @see /dotcom-rendering/src/components/ConfigContext.tsx + */ + const configContext = { renderingTarget }; return ( - + { renderingTarget={renderingTarget} /> )} - + ); }; diff --git a/dotcom-rendering/src/components/BylineLink.tsx b/dotcom-rendering/src/components/BylineLink.tsx index 624c72fecbe..0fa190779c0 100644 --- a/dotcom-rendering/src/components/BylineLink.tsx +++ b/dotcom-rendering/src/components/BylineLink.tsx @@ -5,9 +5,9 @@ import { isContributor, } from '../lib/byline'; import type { TagType } from '../types/tag'; +import { useConfig } from './ConfigContext'; import { FollowWrapper } from './FollowWrapper.importable'; import { Island } from './Island'; -import { useRenderingContext } from './RenderingContext'; type Props = { byline: string; @@ -131,12 +131,12 @@ export const BylineLink = ({ byline, tags, format }: Props) => { ); }); - const { isApps } = useRenderingContext(); + const { renderingTarget } = useConfig(); return ( <> {renderedTokens} - {isApps && soleContributor !== undefined ? ( + {renderingTarget === 'Apps' && soleContributor !== undefined ? ( (undefined); + +export const ConfigProvider = ({ + value, + children, +}: { + value: ApplicationConfig; + children: React.ReactNode; +}) => {children}; + +/** + * useContext hook for safely fetching application configuration + * Ensures that it is used within the relevant Context.Provider + * + * @returns {ConfigContext} + */ +export const useConfig = (): ApplicationConfig => { + const context = useContext(ConfigContext); + + if (context === undefined) { + throw Error('useConfig must be used within the ConfigContext provider'); + } + + // TODO - remove console.log + // console.log('\n🕹️ `useConfig` => ', context); + + return context; +}; diff --git a/dotcom-rendering/src/components/Island.tsx b/dotcom-rendering/src/components/Island.tsx index df8fc7ad0b4..fc8129f9d2f 100644 --- a/dotcom-rendering/src/components/Island.tsx +++ b/dotcom-rendering/src/components/Island.tsx @@ -1,3 +1,4 @@ +import { useConfig } from './ConfigContext'; import { Placeholder } from './Placeholder'; /** @@ -133,18 +134,23 @@ export const Island = ({ placeholderHeight, rootMargin, children, -}: Props) => ( - - {decideChildren(children, clientOnly, placeholderHeight)} - -); +}: Props) => { + const config = useConfig(); + + return ( + + {decideChildren(children, clientOnly, placeholderHeight)} + + ); +}; /** * If JavaScript is disabled, hide client-only islands diff --git a/dotcom-rendering/src/components/RenderingContext.tsx b/dotcom-rendering/src/components/RenderingContext.tsx deleted file mode 100644 index 4946524a9fe..00000000000 --- a/dotcom-rendering/src/components/RenderingContext.tsx +++ /dev/null @@ -1,67 +0,0 @@ -import { createContext, useContext, useMemo } from 'react'; -import type { - EnhancedRenderingContextType, - RenderingContextType, -} from '../types/renderingContext'; - -/** - * Context for global, static values (generic) - * - * This should not contain anything which will change between re-renders. - * It is deliberately set with a default context of `undefined` to better - * surface errors relating to incorrect usage. - * - * It is deliberately not exported - * @see https://kentcdodds.com/blog/how-to-use-react-context-effectively - */ -const RenderingContext = createContext( - undefined, -); - -export const RenderingContextProvider = ({ - value, - children, -}: { - value: RenderingContextType; - children: React.ReactNode; -}) => { - // useMemo aims to reduce unnecessary re-renders for context - const memoisedValue = useMemo(() => value, [value]); - - return ( - - {children} - - ); -}; - -/** - * useContext hook for safely fetching the rendering context value - * Ensures that it is used within a relevant Context.Provider - * - * @example - * const { isWeb } = useRenderingContext(); - * - * if (isWeb) { - * ... - * } else { - * ... - * } - */ -export const useRenderingContext = (): EnhancedRenderingContextType => { - const context = useContext(RenderingContext); - - if (context === undefined) { - throw Error( - 'useRenderingContext must be used within the RenderingContext provider', - ); - } - - const { target } = context; - - return { - target, - isWeb: target === 'Web', - isApps: target === 'Apps', - }; -}; diff --git a/dotcom-rendering/src/lib/ArticleRenderer.tsx b/dotcom-rendering/src/lib/ArticleRenderer.tsx index 41d9c1828e8..3175424fa2f 100644 --- a/dotcom-rendering/src/lib/ArticleRenderer.tsx +++ b/dotcom-rendering/src/lib/ArticleRenderer.tsx @@ -2,7 +2,7 @@ import { css } from '@emotion/react'; import type { ArticleFormat } from '@guardian/libs'; import { ArticleDesign } from '@guardian/libs'; import { adContainerStyles } from '../components/AdSlot'; -import { useRenderingContext } from '../components/RenderingContext'; +import { useConfig } from '../components/ConfigContext'; import { interactiveLegacyClasses } from '../layouts/lib/interactiveLegacyStyling'; import type { ServerSideTests, Switches } from '../types/config'; import type { FEElement } from '../types/content'; @@ -84,7 +84,7 @@ export const ArticleRenderer = ({ ); }); - const { isApps } = useRenderingContext(); + const { renderingTarget } = useConfig(); // const cleanedElements = elements.map(element => // 'html' in element ? { ...element, html: clean(element.html) } : element, @@ -105,7 +105,7 @@ export const ArticleRenderer = ({ ].join(' ')} css={[adStylesDynamic, commercialPosition]} > - {isApps + {renderingTarget === 'Apps' ? renderedElements : /* Insert the placeholder for the sign in gate on the 2nd article element */ withSignInGateSlot({ diff --git a/dotcom-rendering/src/types/configContext.ts b/dotcom-rendering/src/types/configContext.ts new file mode 100644 index 00000000000..017cc43384d --- /dev/null +++ b/dotcom-rendering/src/types/configContext.ts @@ -0,0 +1,11 @@ +import type { RenderingTarget } from './renderingTarget'; + +/** + * Context for global, static, immutable values i.e. application configuration + * + * This should not contain any properties which are likely to change between re-renders + * @see /dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md + */ +export interface ApplicationConfig { + renderingTarget: RenderingTarget; +} diff --git a/dotcom-rendering/src/types/renderingContext.ts b/dotcom-rendering/src/types/renderingContext.ts deleted file mode 100644 index de191c51d78..00000000000 --- a/dotcom-rendering/src/types/renderingContext.ts +++ /dev/null @@ -1,19 +0,0 @@ -import type { RenderingTarget } from './renderingTarget'; - -/** - * Context for global values (generic) - * - * Do not add properties which are likely to change between re-renders - * @see /dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md - */ -export interface RenderingContextType { - target: RenderingTarget; -} - -/** - * Enhanced context based on provided values, making assertions easier - */ -export interface EnhancedRenderingContextType extends RenderingContextType { - isApps: boolean; - isWeb: boolean; -} diff --git a/dotcom-rendering/stories/generated/Layout.stories.tsx b/dotcom-rendering/stories/generated/Layout.stories.tsx index 41650a007b1..d2c3d22f621 100644 --- a/dotcom-rendering/stories/generated/Layout.stories.tsx +++ b/dotcom-rendering/stories/generated/Layout.stories.tsx @@ -28,7 +28,7 @@ export const WebStandardStandardNewsPillar = () => { ); }; WebStandardStandardNewsPillar.storyName = 'Web: Display: Standard, Design: Standard, Theme: NewsPillar'; -WebStandardStandardNewsPillar.args = { renderingContext: { target: 'Web' } }; +WebStandardStandardNewsPillar.args = { config: { renderingTarget: 'Web' } }; export const AppsStandardStandardNewsPillar = () => { return ( @@ -41,7 +41,7 @@ export const AppsStandardStandardNewsPillar = () => { ); }; AppsStandardStandardNewsPillar.storyName = 'Apps: Display: Standard, Design: Standard, Theme: NewsPillar'; -AppsStandardStandardNewsPillar.args = { renderingContext: { target: 'Apps' } }; +AppsStandardStandardNewsPillar.args = { config: { renderingTarget: 'Apps' } }; export const WebShowcasePictureOpinionPillar = () => { return ( @@ -54,4 +54,4 @@ export const WebShowcasePictureOpinionPillar = () => { ); }; WebShowcasePictureOpinionPillar.storyName = 'Web: Display: Showcase, Design: Picture, Theme: OpinionPillar'; -WebShowcasePictureOpinionPillar.args = { renderingContext: { target: 'Web' } }; +WebShowcasePictureOpinionPillar.args = { config: { renderingTarget: 'Web' } }; From 4bf6dec50aacdfc2eb60e6c9b7d2fb7cd25b0672 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 6 Sep 2023 17:02:48 +0100 Subject: [PATCH 11/20] test: add test for config context and mock for other tests --- dotcom-rendering/scripts/jest/setup.ts | 8 ++++ .../src/components/ConfigContext.test.tsx | 45 +++++++++++++++++++ .../src/components/ConfigContext.tsx | 3 -- 3 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 dotcom-rendering/src/components/ConfigContext.test.tsx diff --git a/dotcom-rendering/scripts/jest/setup.ts b/dotcom-rendering/scripts/jest/setup.ts index dca8399e46e..05ee3029076 100644 --- a/dotcom-rendering/scripts/jest/setup.ts +++ b/dotcom-rendering/scripts/jest/setup.ts @@ -112,3 +112,11 @@ global.TextDecoder = TextDecoder as unknown as typeof global.TextDecoder; // Mocks the version number used by CDK, we don't want our tests to fail every time we update our cdk dependency. jest.mock('@guardian/cdk/lib/constants/tracking-tag'); + +// Mocks the useConfig hook in ConfigContext, so that we don't have to use the provider all the time +jest.mock('../../src/components/ConfigContext.tsx', () => { + const mockConfig = { renderingTarget: 'Web' }; + return { + useConfig: () => mockConfig, + }; +}); diff --git a/dotcom-rendering/src/components/ConfigContext.test.tsx b/dotcom-rendering/src/components/ConfigContext.test.tsx new file mode 100644 index 00000000000..85b40baca79 --- /dev/null +++ b/dotcom-rendering/src/components/ConfigContext.test.tsx @@ -0,0 +1,45 @@ +import { render } from '@testing-library/react'; + +// This file is globally mocked in "/dotcom-rendering/scripts/jest/setup.ts" +// so we need to explicitly override this in order to test its functionality +const { useConfig, ConfigProvider } = jest.requireActual('./ConfigContext.tsx'); + +const testId = 'testId'; +const TestComponent = () => { + const config = useConfig(); + return ( +
+

{JSON.stringify(config)}

+
+ ); +}; + +describe('ConfigContext', () => { + describe('without ConfigProvider', () => { + it('does not allow use of the useConfig hook without being inside ConfigProvider', () => { + expect(() => TestComponent()).toThrowError(); + }); + }); + + describe('with ConfigProvider', () => { + it.each(['Web', 'Apps'])( + 'provides correct context through useConfig hook with renderingTarget: "%s"', + (renderingTarget) => { + const config = { renderingTarget }; + const Component = () => { + return ( + + + + ); + }; + const { getByTestId, getByText } = render(); + + expect(getByTestId(testId)).toBeInTheDocument(); + expect(getByText(/renderingTarget/)).toHaveTextContent( + renderingTarget, + ); + }, + ); + }); +}); diff --git a/dotcom-rendering/src/components/ConfigContext.tsx b/dotcom-rendering/src/components/ConfigContext.tsx index 66fadea0f9b..6074e262265 100644 --- a/dotcom-rendering/src/components/ConfigContext.tsx +++ b/dotcom-rendering/src/components/ConfigContext.tsx @@ -34,8 +34,5 @@ export const useConfig = (): ApplicationConfig => { throw Error('useConfig must be used within the ConfigContext provider'); } - // TODO - remove console.log - // console.log('\n🕹️ `useConfig` => ', context); - return context; }; From 9d347717fdb6fdcfd376adc892ffb3d3f5fdf9d8 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 6 Sep 2023 17:06:53 +0100 Subject: [PATCH 12/20] ignore linting issue for context with comment --- dotcom-rendering/src/components/ContentABTest.amp.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/dotcom-rendering/src/components/ContentABTest.amp.tsx b/dotcom-rendering/src/components/ContentABTest.amp.tsx index 8ab92291537..ee9fcb5bcc7 100644 --- a/dotcom-rendering/src/components/ContentABTest.amp.tsx +++ b/dotcom-rendering/src/components/ContentABTest.amp.tsx @@ -58,6 +58,7 @@ export const ContentABTestProvider = ({ children: React.ReactNode; }) => { const group = getGroup(pageId); + // eslint-disable-next-line react/jsx-no-constructed-context-values -- TODO consider using useMemo? const providerValue = switches.ampContentAbTesting ? { group } : {}; return ( {children} From 9162e026711290c004eedfce10f0677e23c60f88 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 6 Sep 2023 17:08:45 +0100 Subject: [PATCH 13/20] copy lint config from AR to detect react version --- dotcom-rendering/.eslintrc.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dotcom-rendering/.eslintrc.js b/dotcom-rendering/.eslintrc.js index 36a83993764..00e206056b7 100644 --- a/dotcom-rendering/.eslintrc.js +++ b/dotcom-rendering/.eslintrc.js @@ -172,6 +172,10 @@ module.exports = { }, settings: { 'import/resolver': 'typescript', + react: { + // Tells eslint-plugin-react to automatically detect the version of React to use + version: 'detect', + }, }, overrides: [ { From 7ae529d8765b53189d86509208d4ea846f46f3e9 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 6 Sep 2023 17:16:10 +0100 Subject: [PATCH 14/20] docs: update react context ADR to remove ban on use within islands, since not practical --- .../docs/architecture/proposed-adrs/react-context-api.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md b/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md index 48baa45fde2..22bfe24e310 100644 --- a/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md +++ b/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md @@ -6,16 +6,15 @@ [This decision was revisited in 2023](https://github.com/guardian/dotcom-rendering/discussions/8696) due to [the introduction of rendering target as a prop](dotcom-rendering/docs/architecture/proposed-adrs/rendering-type.md) which represents a global state, resulting in very heavy prop-drilling throughout the app. This began to complicate pull requests due to so many unrelated changes appearing in the diff since the props needed to be provided at every layer of the app, as well as tightly coupling components unnecessarily. -An RFC was put together [here](https://github.com/guardian/dotcom-rendering/pull/8704) to trial using the React context API for this specific type of global state, which doesn't change throughout the component lifecycle. +An RFC was put together [here](https://github.com/guardian/dotcom-rendering/pull/8704) to trial using the React context API for this specific type of global state, which doesn't change throughout the component lifecycle ie. immutable application configuration. ## Decision The decision to allow use of context more generally rests on the following _"lines in the sand"_: -- Context should be considered **global** for rendered components in dotcom-rendering +- Context should be considered **global, static, and immutable** for rendered components in dotcom-rendering. It can act as a type of configuration for our application. - Context should **only be used for JSX components** (ie. not for JSON responses or non-JSX helper code) -- Context should **not be used for values that change between re-renders**, since this adds unnecessary complexity and there are alternative solutions for these cases -- Context should **only be used for server-side rendered (SSR) components** and therefore should **not be used inside islands** +- Context should **not be used for values that change between re-renders**, since this adds unnecessary complexity and there are alternative solutions for these cases. There is a eslint rule to attempt to prevent this (`react/jsx-no-constructed-context-values`) ## Status From 55a5b215066d5fb77069ef05631e255c6aedb3a2 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Wed, 6 Sep 2023 17:56:17 +0100 Subject: [PATCH 15/20] improve docs, and readability by renaming config type --- .../src/client/islands/doHydration.tsx | 4 ++-- .../src/client/islands/getConfig.ts | 6 +++--- dotcom-rendering/src/components/BylineLink.tsx | 5 +++++ .../src/components/ConfigContext.tsx | 18 ++++++++++-------- dotcom-rendering/src/components/Island.tsx | 7 +++++++ dotcom-rendering/src/lib/ArticleRenderer.tsx | 5 +++++ dotcom-rendering/src/types/configContext.ts | 2 +- 7 files changed, 33 insertions(+), 14 deletions(-) diff --git a/dotcom-rendering/src/client/islands/doHydration.tsx b/dotcom-rendering/src/client/islands/doHydration.tsx index 80f4e40b172..a6d65442565 100644 --- a/dotcom-rendering/src/client/islands/doHydration.tsx +++ b/dotcom-rendering/src/client/islands/doHydration.tsx @@ -5,7 +5,7 @@ import { log, startPerformanceMeasure } from '@guardian/libs'; import { createElement } from 'react'; import { createRoot, hydrateRoot } from 'react-dom/client'; import { ConfigProvider } from '../../components/ConfigContext'; -import type { ApplicationConfig } from '../../types/configContext'; +import type { Config } from '../../types/configContext'; declare global { interface DOMStringMap { @@ -35,7 +35,7 @@ export const doHydration = async ( data: { [key: string]: unknown } | null, element: HTMLElement, emotionCache: EmotionCache, - config: ApplicationConfig, + config: Config, ): Promise => { // If this function has already been run for an element then don't try to // run it a second time diff --git a/dotcom-rendering/src/client/islands/getConfig.ts b/dotcom-rendering/src/client/islands/getConfig.ts index 52460ecc43f..a6fdef28848 100644 --- a/dotcom-rendering/src/client/islands/getConfig.ts +++ b/dotcom-rendering/src/client/islands/getConfig.ts @@ -1,4 +1,4 @@ -import type { ApplicationConfig } from '../../types/configContext'; +import type { Config } from '../../types/configContext'; /** * getConfig takes the given html element and returns its config attribute @@ -8,14 +8,14 @@ import type { ApplicationConfig } from '../../types/configContext'; * @param marker : The html element that we want to read the config attribute from; * @returns */ -export const getConfig = (marker: HTMLElement): ApplicationConfig => { +export const getConfig = (marker: HTMLElement): Config => { const serialised = marker.getAttribute('config'); try { if (!serialised) { throw Error('Unable to fetch config attribute from marker element'); } else { - return JSON.parse(serialised) as ApplicationConfig; + return JSON.parse(serialised) as Config; } } catch (error: unknown) { console.error( diff --git a/dotcom-rendering/src/components/BylineLink.tsx b/dotcom-rendering/src/components/BylineLink.tsx index 0fa190779c0..ae4ab9a9f45 100644 --- a/dotcom-rendering/src/components/BylineLink.tsx +++ b/dotcom-rendering/src/components/BylineLink.tsx @@ -131,6 +131,11 @@ export const BylineLink = ({ byline, tags, format }: Props) => { ); }); + /** + * Where is this coming from? + * Config value is set at high in the component tree within a React context + * @see /dotcom-rendering/src/components/ArticlePage.tsx or look for a `` + */ const { renderingTarget } = useConfig(); return ( diff --git a/dotcom-rendering/src/components/ConfigContext.tsx b/dotcom-rendering/src/components/ConfigContext.tsx index 6074e262265..468e7a898a7 100644 --- a/dotcom-rendering/src/components/ConfigContext.tsx +++ b/dotcom-rendering/src/components/ConfigContext.tsx @@ -1,33 +1,35 @@ import { createContext, useContext } from 'react'; -import type { ApplicationConfig } from '../types/configContext'; +import type { Config } from '../types/configContext'; /** * Context for global, static, immutable values (application configuration) - * * This should not contain anything which will change between re-renders. + * * It is deliberately set with a default context of `undefined` to better * surface errors relating to incorrect usage. * * It is deliberately not exported * @see https://kentcdodds.com/blog/how-to-use-react-context-effectively */ -const ConfigContext = createContext(undefined); +const ConfigContext = createContext(undefined); +/** + * ConfigProvider component should be included at a high level + * so that lower down components can access the context value + */ export const ConfigProvider = ({ value, children, }: { - value: ApplicationConfig; + value: Config; children: React.ReactNode; }) => {children}; /** - * useContext hook for safely fetching application configuration + * useContext hook for safely fetching application configuration. * Ensures that it is used within the relevant Context.Provider - * - * @returns {ConfigContext} */ -export const useConfig = (): ApplicationConfig => { +export const useConfig = (): Config => { const context = useContext(ConfigContext); if (context === undefined) { diff --git a/dotcom-rendering/src/components/Island.tsx b/dotcom-rendering/src/components/Island.tsx index fc8129f9d2f..35196eb1617 100644 --- a/dotcom-rendering/src/components/Island.tsx +++ b/dotcom-rendering/src/components/Island.tsx @@ -135,6 +135,13 @@ export const Island = ({ rootMargin, children, }: Props) => { + /** + * Where is this coming from? + * Config value is set at high in the component tree within a React context + * @see /dotcom-rendering/src/components/ArticlePage.tsx or look for a `` + * + * This is here so that we can provide the config information to the hydrated, client-side rendered components + */ const config = useConfig(); return ( diff --git a/dotcom-rendering/src/lib/ArticleRenderer.tsx b/dotcom-rendering/src/lib/ArticleRenderer.tsx index 1d1dbcbb846..a949b63e452 100644 --- a/dotcom-rendering/src/lib/ArticleRenderer.tsx +++ b/dotcom-rendering/src/lib/ArticleRenderer.tsx @@ -84,6 +84,11 @@ export const ArticleRenderer = ({ ); }); + /** + * Where is this coming from? + * Config value is set at high in the component tree within a React context + * @see /dotcom-rendering/src/components/ArticlePage.tsx or look for a `` + */ const { renderingTarget } = useConfig(); // const cleanedElements = elements.map(element => diff --git a/dotcom-rendering/src/types/configContext.ts b/dotcom-rendering/src/types/configContext.ts index 017cc43384d..98579c0a438 100644 --- a/dotcom-rendering/src/types/configContext.ts +++ b/dotcom-rendering/src/types/configContext.ts @@ -6,6 +6,6 @@ import type { RenderingTarget } from './renderingTarget'; * This should not contain any properties which are likely to change between re-renders * @see /dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md */ -export interface ApplicationConfig { +export interface Config { renderingTarget: RenderingTarget; } From 40af5aa2e070abdbb9e65ab499b40b81b18e08a9 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Thu, 7 Sep 2023 11:52:26 +0100 Subject: [PATCH 16/20] move provider higher up the tree and include other pages --- .../src/components/ArticlePage.tsx | 203 ++++++++---------- .../src/components/BylineLink.tsx | 3 +- dotcom-rendering/src/components/FrontPage.tsx | 3 +- dotcom-rendering/src/components/Island.tsx | 3 +- dotcom-rendering/src/lib/ArticleRenderer.tsx | 3 +- ...render.allEditorialNewslettersPage.web.tsx | 15 +- .../src/server/render.article.amp.tsx | 10 +- .../src/server/render.article.apps.tsx | 17 +- .../src/server/render.article.web.tsx | 76 ++++--- .../src/server/render.front.web.tsx | 16 +- 10 files changed, 191 insertions(+), 158 deletions(-) diff --git a/dotcom-rendering/src/components/ArticlePage.tsx b/dotcom-rendering/src/components/ArticlePage.tsx index 24c79c5bc3c..bff32f25ad8 100644 --- a/dotcom-rendering/src/components/ArticlePage.tsx +++ b/dotcom-rendering/src/components/ArticlePage.tsx @@ -10,7 +10,6 @@ import type { FEArticleType } from '../types/frontend'; import type { RenderingTarget } from '../types/renderingTarget'; import { AlreadyVisited } from './AlreadyVisited.importable'; import { BrazeMessaging } from './BrazeMessaging.importable'; -import { ConfigProvider } from './ConfigContext'; import { FocusStyles } from './FocusStyles.importable'; import { Island } from './Island'; import { LightboxHash } from './LightboxHash.importable'; @@ -41,8 +40,8 @@ interface AppProps extends BaseProps { /** * @description * Article is a high level wrapper for article pages on Dotcom. - * Sets strict mode and some globals, as well as providing high-level context. - * */ + * Sets strict mode and some globals, as well as providing config via context. + */ export const ArticlePage = (props: WebProps | AppProps) => { const { article, format, renderingTarget } = props; @@ -55,121 +54,107 @@ export const ArticlePage = (props: WebProps | AppProps) => { adUnit: article.config.adUnit, }); - /** - * The static, immutable context value for the app - * @see /dotcom-rendering/src/components/ConfigContext.tsx - */ - const configContext = { renderingTarget }; - return ( - - - - - {article.config.switches.lightbox && - article.imagesForLightbox && ( - <> - - - - - - - - - )} - - - - {(format.design === ArticleDesign.LiveBlog || - format.design === ArticleDesign.DeadBlog) && ( - + + + {article.config.switches.lightbox && article.imagesForLightbox && ( + <> + - )} - {renderingTarget === 'Web' && ( - <> - - - - - - - - - - - - - - - - - - )} - {renderingTarget === 'Web' ? ( - + - ) : ( - - + + + + )} + + + + {(format.design === ArticleDesign.LiveBlog || + format.design === ArticleDesign.DeadBlog) && ( + + )} + {renderingTarget === 'Web' && ( + <> + + + + + + - )} - {renderingTarget === 'Apps' ? ( - - ) : ( - + + + + + + + + + + )} + {renderingTarget === 'Web' ? ( + + + + ) : ( + + - )} - + + )} + {renderingTarget === 'Apps' ? ( + + ) : ( + + )} ); }; diff --git a/dotcom-rendering/src/components/BylineLink.tsx b/dotcom-rendering/src/components/BylineLink.tsx index ae4ab9a9f45..dd074021507 100644 --- a/dotcom-rendering/src/components/BylineLink.tsx +++ b/dotcom-rendering/src/components/BylineLink.tsx @@ -133,8 +133,7 @@ export const BylineLink = ({ byline, tags, format }: Props) => { /** * Where is this coming from? - * Config value is set at high in the component tree within a React context - * @see /dotcom-rendering/src/components/ArticlePage.tsx or look for a `` + * Config value is set at high in the component tree within a React context in a `` */ const { renderingTarget } = useConfig(); diff --git a/dotcom-rendering/src/components/FrontPage.tsx b/dotcom-rendering/src/components/FrontPage.tsx index 6859729bed7..63c26ff7327 100644 --- a/dotcom-rendering/src/components/FrontPage.tsx +++ b/dotcom-rendering/src/components/FrontPage.tsx @@ -24,7 +24,8 @@ type Props = { /** * @description - * FrontPage is a high level wrapper for front pages on Dotcom. Sets strict mode and some globals + * FrontPage is a high level wrapper for front pages on Dotcom. + * Sets strict mode and some globals, as well as providing config via context. * * @param {Props} props * @param {DCRFrontType} props.front - The article JSON data diff --git a/dotcom-rendering/src/components/Island.tsx b/dotcom-rendering/src/components/Island.tsx index 35196eb1617..e80684d0aae 100644 --- a/dotcom-rendering/src/components/Island.tsx +++ b/dotcom-rendering/src/components/Island.tsx @@ -137,8 +137,7 @@ export const Island = ({ }: Props) => { /** * Where is this coming from? - * Config value is set at high in the component tree within a React context - * @see /dotcom-rendering/src/components/ArticlePage.tsx or look for a `` + * Config value is set at high in the component tree within a React context in a `` * * This is here so that we can provide the config information to the hydrated, client-side rendered components */ diff --git a/dotcom-rendering/src/lib/ArticleRenderer.tsx b/dotcom-rendering/src/lib/ArticleRenderer.tsx index a949b63e452..ce404c87ee6 100644 --- a/dotcom-rendering/src/lib/ArticleRenderer.tsx +++ b/dotcom-rendering/src/lib/ArticleRenderer.tsx @@ -86,8 +86,7 @@ export const ArticleRenderer = ({ /** * Where is this coming from? - * Config value is set at high in the component tree within a React context - * @see /dotcom-rendering/src/components/ArticlePage.tsx or look for a `` + * Config value is set at high in the component tree within a React context in a `` */ const { renderingTarget } = useConfig(); diff --git a/dotcom-rendering/src/server/render.allEditorialNewslettersPage.web.tsx b/dotcom-rendering/src/server/render.allEditorialNewslettersPage.web.tsx index f8f87b11f4b..f5485545767 100644 --- a/dotcom-rendering/src/server/render.allEditorialNewslettersPage.web.tsx +++ b/dotcom-rendering/src/server/render.allEditorialNewslettersPage.web.tsx @@ -1,4 +1,5 @@ import { AllEditorialNewslettersPage } from '../components/AllEditorialNewslettersPage'; +import { ConfigProvider } from '../components/ConfigContext'; import { generateScriptTags, getModulesBuild, @@ -9,6 +10,7 @@ import { getHttp3Url } from '../lib/getHttp3Url'; import { polyfillIO } from '../lib/polyfill.io'; import { extractNAV } from '../model/extract-nav'; import { createGuardian } from '../model/guardian'; +import type { Config } from '../types/configContext'; import type { DCRNewslettersPageType } from '../types/newslettersPage'; import { htmlPageTemplate } from './htmlPageTemplate'; @@ -22,11 +24,16 @@ export const renderEditorialNewslettersPage = ({ const title = newslettersPage.webTitle; const NAV = extractNAV(newslettersPage.nav); + // The newsletters page is currently only supported on Web + const config: Config = { renderingTarget: 'Web' }; + const { html, extractedCss } = renderToStringWithEmotion( - , + + + , ); // Evaluating the performance of HTTP3 over HTTP2 diff --git a/dotcom-rendering/src/server/render.article.amp.tsx b/dotcom-rendering/src/server/render.article.amp.tsx index a4139fb21cf..de3f90ef278 100644 --- a/dotcom-rendering/src/server/render.article.amp.tsx +++ b/dotcom-rendering/src/server/render.article.amp.tsx @@ -5,9 +5,11 @@ import { resets } from '@guardian/source-foundations'; import he from 'he'; import React from 'react'; import { renderToStaticMarkup } from 'react-dom/server'; +import { ConfigProvider } from '../components/ConfigContext'; import { epicChoiceCardCss } from '../components/Epic.amp'; import { stickyAdLabelCss } from '../components/StickyAd.amp'; import { getFontsCss } from '../lib/fonts-css'; +import type { Config } from '../types/configContext'; interface RenderToStringResult { html: string; @@ -36,9 +38,15 @@ export const renderArticle = ({ const cache = createCache({ key }); // eslint-disable-next-line @typescript-eslint/unbound-method const { extractCritical } = createEmotionServer(cache); + + // We are currently considering AMP to be a renderingTarget of Web + const config: Config = { renderingTarget: 'Web' }; + const { html, css }: RenderToStringResult = extractCritical( renderToStaticMarkup( - {body}, + + {body} + , ), ); diff --git a/dotcom-rendering/src/server/render.article.apps.tsx b/dotcom-rendering/src/server/render.article.apps.tsx index c031c6cf2d7..28bab1f545e 100644 --- a/dotcom-rendering/src/server/render.article.apps.tsx +++ b/dotcom-rendering/src/server/render.article.apps.tsx @@ -1,9 +1,11 @@ import { isString } from '@guardian/libs'; import { ArticlePage } from '../components/ArticlePage'; +import { ConfigProvider } from '../components/ConfigContext'; import { generateScriptTags, getPathFromManifest } from '../lib/assets'; import { decideFormat } from '../lib/decideFormat'; import { renderToStringWithEmotion } from '../lib/emotion'; import { createGuardian } from '../model/guardian'; +import type { Config } from '../types/configContext'; import type { FEArticleType } from '../types/frontend'; import { htmlPageTemplate } from './htmlPageTemplate'; @@ -15,12 +17,17 @@ export const renderArticle = ( } => { const format: ArticleFormat = decideFormat(article.format); + const renderingTarget = 'Apps'; + const config: Config = { renderingTarget }; + const { html, extractedCss } = renderToStringWithEmotion( - , + + + , ); const clientScripts = [getPathFromManifest('apps', 'index.js')]; diff --git a/dotcom-rendering/src/server/render.article.web.tsx b/dotcom-rendering/src/server/render.article.web.tsx index ddf722b2f2c..b1767d5ad4e 100644 --- a/dotcom-rendering/src/server/render.article.web.tsx +++ b/dotcom-rendering/src/server/render.article.web.tsx @@ -1,5 +1,6 @@ import { ArticleDesign, isString, Pillar } from '@guardian/libs'; import { ArticlePage } from '../components/ArticlePage'; +import { ConfigProvider } from '../components/ConfigContext'; import { isAmpSupported } from '../components/Elements.amp'; import { KeyEventsContainer } from '../components/KeyEventsContainer'; import { @@ -18,6 +19,7 @@ import { polyfillIO } from '../lib/polyfill.io'; import { extractGA } from '../model/extract-ga'; import { extractNAV } from '../model/extract-nav'; import { createGuardian as createWindowGuardian } from '../model/guardian'; +import type { Config } from '../types/configContext'; import type { FEElement } from '../types/content'; import type { FEArticleType, FEBlocksRequest } from '../types/frontend'; import type { TagType } from '../types/tag'; @@ -47,13 +49,18 @@ export const renderHtml = ({ const format: ArticleFormat = decideFormat(article.format); + const renderingTarget = 'Web'; + const config: Config = { renderingTarget }; + const { html, extractedCss } = renderToStringWithEmotion( - , + + + , ); // We want to only insert script tags for the elements or main media elements on this page view @@ -248,26 +255,31 @@ export const renderBlocks = ({ }: FEBlocksRequest): string => { const format: ArticleFormat = decideFormat(FEFormat); + // Only currently supported for Web + const config: Config = { renderingTarget: 'Web' }; + const { html, extractedCss } = renderToStringWithEmotion( - , + + + , ); return `${extractedCss}${html}`; @@ -284,12 +296,16 @@ export const renderKeyEvents = ({ format: FEFormat, filterKeyEvents, }: FEKeyEventsRequest): string => { + const config: Config = { renderingTarget: 'Web' }; + const { html, extractedCss } = renderToStringWithEmotion( - , + + + , ); return `${extractedCss}${html}`; diff --git a/dotcom-rendering/src/server/render.front.web.tsx b/dotcom-rendering/src/server/render.front.web.tsx index 98fe5442622..157491a6d59 100644 --- a/dotcom-rendering/src/server/render.front.web.tsx +++ b/dotcom-rendering/src/server/render.front.web.tsx @@ -1,4 +1,5 @@ import { isString, Pillar } from '@guardian/libs'; +import { ConfigProvider } from '../components/ConfigContext'; import { FrontPage } from '../components/FrontPage'; import { TagFrontPage } from '../components/TagFrontPage'; import { @@ -13,6 +14,7 @@ import { themeToPillar } from '../lib/themeToPillar'; import type { NavType } from '../model/extract-nav'; import { extractNAV } from '../model/extract-nav'; import { createGuardian } from '../model/guardian'; +import type { Config } from '../types/configContext'; import type { DCRFrontType } from '../types/front'; import type { DCRTagFrontType } from '../types/tagFront'; import { htmlPageTemplate } from './htmlPageTemplate'; @@ -75,8 +77,13 @@ export const renderFront = ({ const title = front.webTitle; const NAV = extractFrontNav(front); + // Fronts are not supported in Apps + const config: Config = { renderingTarget: 'Web' }; + const { html, extractedCss } = renderToStringWithEmotion( - , + + , + , ); // Evaluating the performance of HTTP3 over HTTP2 @@ -164,8 +171,13 @@ export const renderTagFront = ({ const title = tagFront.webTitle; const NAV = extractNAV(tagFront.nav); + // Fronts are not supported in Apps + const config: Config = { renderingTarget: 'Web' }; + const { html, extractedCss } = renderToStringWithEmotion( - , + + , + , ); // Evaluating the performance of HTTP3 over HTTP2 From 06b26dfbe524ffd9020e18ac8f47b9304c4890a7 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Thu, 7 Sep 2023 14:57:13 +0100 Subject: [PATCH 17/20] test: fix tests --- dotcom-rendering/scripts/jest/setup.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/dotcom-rendering/scripts/jest/setup.ts b/dotcom-rendering/scripts/jest/setup.ts index 886bf3b0288..299a0b66617 100644 --- a/dotcom-rendering/scripts/jest/setup.ts +++ b/dotcom-rendering/scripts/jest/setup.ts @@ -118,6 +118,7 @@ jest.mock('@guardian/cdk/lib/constants/tracking-tag'); jest.mock('../../src/components/ConfigContext.tsx', () => { const mockConfig = { renderingTarget: 'Web' }; return { + ...jest.requireActual('../../src/components/ConfigContext.tsx'), useConfig: () => mockConfig, }; }); From 2a0d92a9520caf285679fa0b9209cb6c7b19476f Mon Sep 17 00:00:00 2001 From: Charlotte Date: Thu, 7 Sep 2023 16:28:59 +0100 Subject: [PATCH 18/20] docs: update jsdoc descriptions for page components --- dotcom-rendering/src/components/ArticlePage.tsx | 3 +-- dotcom-rendering/src/components/FrontPage.tsx | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/dotcom-rendering/src/components/ArticlePage.tsx b/dotcom-rendering/src/components/ArticlePage.tsx index bff32f25ad8..fa5007a0fde 100644 --- a/dotcom-rendering/src/components/ArticlePage.tsx +++ b/dotcom-rendering/src/components/ArticlePage.tsx @@ -39,8 +39,7 @@ interface AppProps extends BaseProps { /** * @description - * Article is a high level wrapper for article pages on Dotcom. - * Sets strict mode and some globals, as well as providing config via context. + * Article is a high level wrapper for article pages on Dotcom. Sets strict mode and some globals */ export const ArticlePage = (props: WebProps | AppProps) => { const { article, format, renderingTarget } = props; diff --git a/dotcom-rendering/src/components/FrontPage.tsx b/dotcom-rendering/src/components/FrontPage.tsx index 63c26ff7327..6859729bed7 100644 --- a/dotcom-rendering/src/components/FrontPage.tsx +++ b/dotcom-rendering/src/components/FrontPage.tsx @@ -24,8 +24,7 @@ type Props = { /** * @description - * FrontPage is a high level wrapper for front pages on Dotcom. - * Sets strict mode and some globals, as well as providing config via context. + * FrontPage is a high level wrapper for front pages on Dotcom. Sets strict mode and some globals * * @param {Props} props * @param {DCRFrontType} props.front - The article JSON data From 5cd1902394086733544f29fa5f46a31aca794ee1 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Thu, 7 Sep 2023 17:11:26 +0100 Subject: [PATCH 19/20] docs: clarify context docs and update status, moving existing doc to historic dir --- .../react-context-api.md => 028-react-context-api.md} | 11 +++++++---- .../{ => historic-adrs}/016-react-context-api.md | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) rename dotcom-rendering/docs/architecture/{proposed-adrs/react-context-api.md => 028-react-context-api.md} (74%) rename dotcom-rendering/docs/architecture/{ => historic-adrs}/016-react-context-api.md (93%) diff --git a/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md b/dotcom-rendering/docs/architecture/028-react-context-api.md similarity index 74% rename from dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md rename to dotcom-rendering/docs/architecture/028-react-context-api.md index 22bfe24e310..afca5054db9 100644 --- a/dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md +++ b/dotcom-rendering/docs/architecture/028-react-context-api.md @@ -12,10 +12,13 @@ An RFC was put together [here](https://github.com/guardian/dotcom-rendering/pull The decision to allow use of context more generally rests on the following _"lines in the sand"_: -- Context should be considered **global, static, and immutable** for rendered components in dotcom-rendering. It can act as a type of configuration for our application. -- Context should **only be used for JSX components** (ie. not for JSON responses or non-JSX helper code) -- Context should **not be used for values that change between re-renders**, since this adds unnecessary complexity and there are alternative solutions for these cases. There is a eslint rule to attempt to prevent this (`react/jsx-no-constructed-context-values`) +- Context should be considered **global, static, and immutable** for rendered components in dotcom-rendering. + - It can be thought of as a type of configuration for our application. +- Context should **not be used for values that change between re-renders**, since this adds unnecessary complexity and there are alternative solutions for these cases. + - There is a eslint rule to attempt to prevent this (`react/jsx-no-constructed-context-values`) +- Context should **only be used for React components** (ie. not for JSON responses or non-JSX helper code) + - This is because the React context API will not work outside of React ## Status -Proposed +Approved diff --git a/dotcom-rendering/docs/architecture/016-react-context-api.md b/dotcom-rendering/docs/architecture/historic-adrs/016-react-context-api.md similarity index 93% rename from dotcom-rendering/docs/architecture/016-react-context-api.md rename to dotcom-rendering/docs/architecture/historic-adrs/016-react-context-api.md index 3fb76ca49b1..8d87cc92b28 100644 --- a/dotcom-rendering/docs/architecture/016-react-context-api.md +++ b/dotcom-rendering/docs/architecture/historic-adrs/016-react-context-api.md @@ -15,4 +15,4 @@ the react context api to extract the `edition` property to prevent this. ## Status -Approved +Superceded by 028-react-context-api.md From 0b278e5f127c05ed82a9c81a5bc44a635078f743 Mon Sep 17 00:00:00 2001 From: Charlotte Date: Fri, 8 Sep 2023 11:16:44 +0100 Subject: [PATCH 20/20] docs: spelling --- .../docs/architecture/historic-adrs/016-react-context-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dotcom-rendering/docs/architecture/historic-adrs/016-react-context-api.md b/dotcom-rendering/docs/architecture/historic-adrs/016-react-context-api.md index 8d87cc92b28..c16a59a3af3 100644 --- a/dotcom-rendering/docs/architecture/historic-adrs/016-react-context-api.md +++ b/dotcom-rendering/docs/architecture/historic-adrs/016-react-context-api.md @@ -15,4 +15,4 @@ the react context api to extract the `edition` property to prevent this. ## Status -Superceded by 028-react-context-api.md +Superseded by 028-react-context-api.md