-
Notifications
You must be signed in to change notification settings - Fork 30
Feast "thrasher" component #15316
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
base: main
Are you sure you want to change the base?
Feast "thrasher" component #15316
Conversation
8c1bab9 to
749bc8b
Compare
| app.post('/AppsBlocks', handleAppsBlocks); | ||
| app.post('/EditionsCrossword', handleEditionsCrossword); | ||
| app.post('/AppsHostedContent', handleAppsHostedContent); | ||
| app.post('/AppsComponent/Thrasher/:name', handleAppsThrasher); |
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.
this could just be a GET endpoint, not sure how much it matters
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.
We might want to POST a payload in future, if/when we make the copy and image configurable
d10cf47 to
6111162
Compare
JamieB-gu
left a comment
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.
Looks great! A few comments and questions.
| import { FeastThrasher } from './FeastThrasher'; | ||
|
|
||
| const meta = { | ||
| title: 'Components/Marketing/Thrashers/FeastThrasher', | ||
| component: FeastThrasher, | ||
| } satisfies Meta<typeof FeastThrasher>; | ||
|
|
||
| export default meta; | ||
| type Story = StoryObj<typeof meta>; | ||
|
|
||
| export const Default: Story = {}; |
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.
Given there's only one story, if you do this it hoists the story in the sidebar1:
| import { FeastThrasher } from './FeastThrasher'; | |
| const meta = { | |
| title: 'Components/Marketing/Thrashers/FeastThrasher', | |
| component: FeastThrasher, | |
| } satisfies Meta<typeof FeastThrasher>; | |
| export default meta; | |
| type Story = StoryObj<typeof meta>; | |
| export const Default: Story = {}; | |
| import { FeastThrasher as FeastThrasherComponent } from './FeastThrasher'; | |
| const meta = { | |
| title: 'Components/Marketing/Thrashers/FeastThrasher', | |
| component: FeastThrasherComponent, | |
| } satisfies Meta<typeof FeastThrasherComponent>; | |
| export default meta; | |
| type Story = StoryObj<typeof meta>; | |
| export const FeastThrasher: Story = {}; |
Footnotes
|
|
||
| export const renderThrasher = (name: ThrasherName): { html: string } => { | ||
| const { html, extractedCss } = renderToStringWithEmotion( | ||
| <div>{name === 'feast' && <FeastThrasher />}</div>, |
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.
What's the reason for checking that the name is "feast" if the type already guarantees this? Also, do you need to wrap this in a div? In other words, could this just be?
| <div>{name === 'feast' && <FeastThrasher />}</div>, | |
| <FeastThrasher />, |
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.
It is redundant - but because this handler is intended to support more thrashers in future (with the ThrasherName type) it felt right to include the check anyway. I'd be ok with simplifying for now though
| SvgGuardianLogo, | ||
| } from '@guardian/source/react-components'; | ||
|
|
||
| const styles = { |
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.
Out of curiosity, where does this pattern of creating a single object to store styles come from?
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.
It's quite common in the marketing components, e.g. banners. I guess the alternative is adding a Styles suffix to various names
| const tabletImageUrl = | ||
| 'https://media.guim.co.uk/2c4014d476310737dce1e830186e5b6fe18d3327/0_0_2000_2000/2000.jpg'; | ||
| const mobileAndDesktopImageUrl = | ||
| 'https://media.guim.co.uk/a0cc02db1394f8710bdce008e2297759098d53b3/0_0_2000_1200/2000.png'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to use the image optimiser to make these images more responsive to different screen sizes and pixel densities? Given this isn't live yet, this could be done in a follow-up change.
dotcom-rendering/dotcom-rendering/src/lib/image.ts
Lines 44 to 51 in f912e23
| /** | |
| * Generates a URL for calling the Fastly Image Optimiser. | |
| * | |
| * @see https://developer.fastly.com/reference/io/ | |
| * @see https://github.com/guardian/fastly-image-service/blob/main/fastly-io_guim_co_uk/src/main/resources/varnish/main.vcl | |
| * | |
| */ | |
| export const generateImageURL = ({ |
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.
These urls are copied over from the existing thrasher in the interactives repo. I'll take a look at the optimiser
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 guess one of the benefits of moving it into dotcom-rendering is that we get to reuse features like this!
Background
The Feast thrasher is displayed on fronts to advertise the Feast app.
Currently it is implemented as an interactive atom in the interactives repo: https://github.com/guardian/interactives/tree/main/thrashers/feast-ny-thrasher
It is added in the Fronts tool as a "snap link" using the CAPI url:

The flow for updating it is:
Problems with this approach:
We'd prefer to build + maintain thrasher components in dotcom-rendering, for use by web and apps.
This change
Adds a new component, FeastThrasher. Includes a storybook entry.
The component looks broadly the same, though it now aligns better with the front layout.
This is served as a stand alone page from a new DCR endpoint,
/AppsComponent/thrasher/feast. This is a very minimal page for use by the apps so that they can render it in a webview, just as we currently do for the interactive atoms. It includes necessary fonts, meaning we no longer need the font-face definitions.We'll need changes to Frontend and Router to expose this endpoint.
For now nothing will actually use this component.
Future PRs will:
https://api.nextgen.guardianapps.co.uk/component/thrasher/feast?dcr=apps) for use by the apps. It's likely there will be some styling fixes needed for apps compatibility, but we need to integration test to get to that point.Later we should also consider how to improve how this is configured in the Fronts tool. But to begin with we can continue using the old snap link as an indication that the FeastThrasher should be used.
Testing
I tested how it looks in a front by temporarily adding code to the FrontLayout (see commit) -
Desktop:

Tablet:

Mobile:
