-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: use React Context to pass server side tests #7126
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
import { ArticleDesign, ArticleDisplay, ArticlePillar } from '@guardian/libs'; | ||
import { ArticleContainer } from './ArticleContainer'; | ||
import { ArticleHeadline } from './ArticleHeadline'; | ||
import { Flex } from './Flex'; | ||
import { LeftColumn } from './LeftColumn'; | ||
import { Section } from './Section'; | ||
import { | ||
mockServerSideTestsProviderFactory, | ||
useServerSideTests, | ||
} from './ServerSideTestProvider'; | ||
|
||
export default { | ||
component: ArticleHeadline, | ||
title: 'Components/ArticleHeadlineServerTestDemo', | ||
}; | ||
|
||
const format = { | ||
display: ArticleDisplay.Standard, | ||
design: ArticleDesign.Standard, | ||
theme: ArticlePillar.News, | ||
}; | ||
|
||
const ArticleHeadlineWithTest = () => { | ||
const { activeServerSideTests } = useServerSideTests(); | ||
return ( | ||
<> | ||
<ArticleHeadline | ||
headlineString="This is how the default headline looks" | ||
format={format} | ||
tags={[]} | ||
webPublicationDateDeprecated="" | ||
/> | ||
{activeServerSideTests.myTest == 'variant' | ||
? '~*^_ the variant version _^*~' | ||
: ''} | ||
</> | ||
); | ||
}; | ||
|
||
export const VariantStory = () => { | ||
return ( | ||
<Section fullWidth={true}> | ||
<Flex> | ||
<LeftColumn borderType="full"> | ||
<></> | ||
</LeftColumn> | ||
<ArticleContainer format={format}> | ||
<ArticleHeadlineWithTest /> | ||
</ArticleContainer> | ||
</Flex> | ||
</Section> | ||
); | ||
}; | ||
VariantStory.story = { | ||
name: 'Variant Demo', | ||
decorators: [mockServerSideTestsProviderFactory({ myTest: 'variant' })], | ||
}; | ||
|
||
export const ControlStory = () => { | ||
return ( | ||
<Section fullWidth={true}> | ||
<Flex> | ||
<LeftColumn borderType="full"> | ||
<></> | ||
</LeftColumn> | ||
<ArticleContainer format={format}> | ||
<ArticleHeadlineWithTest /> | ||
</ArticleContainer> | ||
</Flex> | ||
</Section> | ||
); | ||
}; | ||
ControlStory.story = { | ||
name: 'Control Demo', | ||
decorators: [mockServerSideTestsProviderFactory({ myTest: 'control' })], | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ import { FocusStyles } from './FocusStyles.importable'; | |
import { Island } from './Island'; | ||
import { Metrics } from './Metrics.importable'; | ||
import { ReaderRevenueDev } from './ReaderRevenueDev.importable'; | ||
import { ServerSideTestProvider } from './ServerSideTestProvider'; | ||
import { SetABTests } from './SetABTests.importable'; | ||
import { SkipTo } from './SkipTo'; | ||
|
||
|
@@ -34,61 +35,70 @@ type Props = { | |
export const ArticlePage = ({ CAPIArticle, NAV, format }: Props) => { | ||
return ( | ||
<StrictMode> | ||
<Global | ||
styles={css` | ||
/* Crude but effective mechanism. Specific components may need to improve on this behaviour. */ | ||
/* The not(.src...) selector is to work with Source's FocusStyleManager. */ | ||
*:focus { | ||
${focusHalo} | ||
} | ||
::selection { | ||
background: ${brandAlt[400]}; | ||
color: ${neutral[7]}; | ||
} | ||
`} | ||
/> | ||
<SkipTo id="maincontent" label="Skip to main content" /> | ||
<SkipTo id="navigation" label="Skip to navigation" /> | ||
{(format.design === ArticleDesign.LiveBlog || | ||
format.design === ArticleDesign.DeadBlog) && ( | ||
<SkipTo id={'key-events-carousel'} label="Skip to key events" /> | ||
)} | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<AlreadyVisited /> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<FocusStyles /> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<Metrics | ||
commercialMetricsEnabled={ | ||
!!CAPIArticle.config.switches.commercialMetrics | ||
} | ||
<ServerSideTestProvider tests={CAPIArticle.config.abTests}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the only significant change in this file should be adding this |
||
<Global | ||
styles={css` | ||
/* Crude but effective mechanism. Specific components may need to improve on this behaviour. */ | ||
/* The not(.src...) selector is to work with Source's FocusStyleManager. */ | ||
*:focus { | ||
${focusHalo} | ||
} | ||
::selection { | ||
background: ${brandAlt[400]}; | ||
color: ${neutral[7]}; | ||
} | ||
`} | ||
/> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<BrazeMessaging idApiUrl={CAPIArticle.config.idApiUrl} /> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<ReaderRevenueDev | ||
shouldHideReaderRevenue={ | ||
CAPIArticle.shouldHideReaderRevenue | ||
} | ||
<SkipTo id="maincontent" label="Skip to main content" /> | ||
<SkipTo id="navigation" label="Skip to navigation" /> | ||
{(format.design === ArticleDesign.LiveBlog || | ||
format.design === ArticleDesign.DeadBlog) && ( | ||
<SkipTo | ||
id={'key-events-carousel'} | ||
label="Skip to key events" | ||
/> | ||
)} | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<AlreadyVisited /> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<FocusStyles /> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<Metrics | ||
commercialMetricsEnabled={ | ||
!!CAPIArticle.config.switches.commercialMetrics | ||
} | ||
/> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<BrazeMessaging idApiUrl={CAPIArticle.config.idApiUrl} /> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<ReaderRevenueDev | ||
shouldHideReaderRevenue={ | ||
CAPIArticle.shouldHideReaderRevenue | ||
} | ||
/> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<FetchCommentCounts repeat={true} /> | ||
</Island> | ||
<Island clientOnly={true}> | ||
<SetABTests | ||
abTestSwitches={filterABTestSwitches( | ||
CAPIArticle.config.switches, | ||
)} | ||
pageIsSensitive={CAPIArticle.config.isSensitive} | ||
isDev={!!CAPIArticle.config.isDev} | ||
/> | ||
</Island> | ||
<DecideLayout | ||
CAPIArticle={CAPIArticle} | ||
NAV={NAV} | ||
format={format} | ||
/> | ||
</Island> | ||
<Island clientOnly={true} deferUntil="idle"> | ||
<FetchCommentCounts repeat={true} /> | ||
</Island> | ||
<Island clientOnly={true}> | ||
<SetABTests | ||
abTestSwitches={filterABTestSwitches( | ||
CAPIArticle.config.switches, | ||
)} | ||
pageIsSensitive={CAPIArticle.config.isSensitive} | ||
isDev={!!CAPIArticle.config.isDev} | ||
/> | ||
</Island> | ||
<DecideLayout CAPIArticle={CAPIArticle} NAV={NAV} format={format} /> | ||
</ServerSideTestProvider> | ||
</StrictMode> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
import type { PartialStoryFn } from '@storybook/addons/dist/ts3.9/types'; | ||
import type { ComponentProps, JSXElementConstructor } from 'react'; | ||
import React from 'react'; | ||
import type { ServerSideTests } from '../../types/config'; | ||
|
||
const ServerSideTestContext = React.createContext< | ||
{ activeServerSideTests: ServerSideTests } | undefined | ||
>(undefined); | ||
|
||
export const ServerSideTestProvider = ({ | ||
tests, | ||
children, | ||
}: { | ||
tests: ServerSideTests; | ||
children: React.ReactNode; | ||
}) => ( | ||
<ServerSideTestContext.Provider value={{ activeServerSideTests: tests }}> | ||
{children} | ||
</ServerSideTestContext.Provider> | ||
); | ||
|
||
/** | ||
* Get current server side tests | ||
* | ||
* @example | ||
* const { activeServerSideTests } = useServerSideTests(); | ||
* | ||
* if (activeServerSideTests.myTestName == "variant") { | ||
* //... | ||
* } else { | ||
* //... | ||
* } | ||
* | ||
*/ | ||
export const useServerSideTests = () => { | ||
const context = React.useContext(ServerSideTestContext); | ||
|
||
if (context === undefined) { | ||
throw Error( | ||
'useServerSideTests must be used within the ContentABTestProvider', | ||
); | ||
} | ||
|
||
return context; | ||
}; | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've adapted the wrapper component and the hook code above from The Storybook decorator is something new that I've come up with today (which probably shows when you read the code!) It feels like Storybook/Chromatic might be one of the main pain points for using Context in this way, so I thought it would be worth trying to create a user-friendly way to mock the Using a factory here doesn't feel ideal tbh. I ended up doing it to make the type declarations easier (for me, at least!) but I'm sure that it could be rewritten in a more user-friendly way if the pattern is adopted. |
||
* | ||
* @param tests | ||
* @returns A Storybook decorator function which will wrap a story in a <ServerSideTestProvider> | ||
* containing the data passed to the factory in the `tests` parameter. | ||
* This decorator should receive a type parameter corresponding to the top-level | ||
* component in the story. | ||
* | ||
* @example | ||
export default { | ||
component: MyComponent, | ||
title: 'Components/MyComponent', | ||
decorators: [ | ||
mockServerSideTestsProviderFactory({myTest: "control"})<typeof MyComponent>, | ||
], | ||
}; | ||
*/ | ||
export function mockServerSideTestsProviderFactory(tests: ServerSideTests) { | ||
return function < | ||
/* eslint-disable @typescript-eslint/no-explicit-any -- Can we narrow this? */ | ||
C extends keyof JSX.IntrinsicElements | JSXElementConstructor<any>, | ||
/* eslint-enable @typescript-eslint/no-explicit-any */ | ||
>(Story: PartialStoryFn<ComponentProps<C>>) { | ||
return ( | ||
<ServerSideTestProvider tests={tests}> | ||
<Story /> | ||
</ServerSideTestProvider> | ||
); | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm creating this component in the story file just to demonstrate what a component with a test would look like. We can imagine that this is a regular component in the
components/
directory which has had test code added to it.