-
Notifications
You must be signed in to change notification settings - Fork 30
Add Storylines content to tag pages #15008
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?
Conversation
27791e1 to
dd106f8
Compare
dd106f8 to
d5fb9f0
Compare
fredex42
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.
This looks great to me. I've noticed a few places where we could zap commented code and a couple of bits marked with eyes that might need another quick look but from a non-DCR expert point of view looking great 👍
| // const sectionHeadlineFromLeftCol = (borderColour: string) => css` | ||
| // ${from.leftCol} { | ||
| // position: relative; | ||
| // ::after { | ||
| // content: ''; | ||
| // display: block; | ||
| // width: 1px; | ||
| // top: 0; | ||
| // height: 1.875rem; | ||
| // right: -10px; | ||
| // position: absolute; | ||
| // background-color: ${borderColour}; | ||
| // } | ||
| // } | ||
| // `; |
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.
zappable?
| dislikeHandler, | ||
| likeHandler, | ||
| }: Props) => { | ||
| const id = sectionId || 'unknown-id'; //todo: figure out what this should be |
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.
👀
| // ${textSans17}; | ||
| // font-weight: 700; |
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.
⚡
| // console.log('StorylinesContent', StorylinesContent); | ||
| // const [storylines, SetStorylines] = useState<StorylinesContent>(); |
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.
⚡
| // useEffect(() => { | ||
| // fetch( | ||
| // `http://localhost:9000/api/tag-page-rendering/${tagPage.pageId}`, | ||
| // { | ||
| // headers: { | ||
| // 'Content-Type': 'application/json', | ||
| // }, | ||
| // credentials: 'include', | ||
| // }, | ||
| // ) | ||
| // .then((response) => { | ||
| // console.log('response', response); | ||
| // return response.json(); | ||
| // }) | ||
| // .then((data) => SetStorylines(data)) | ||
| // .catch((error) => { | ||
| // console.error('Error fetching storylines data:', error); | ||
| // }); | ||
| // }, []); |
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.
⚡
| image={tagPage.header.image} | ||
| /> | ||
| {tagPage.groupedTrails.map((groupedTrails, index) => { | ||
| // console.log("groupedTrails in TagPageLayout:", groupedTrails); |
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.
⚡
|
|
||
| const insertSCSection = | ||
| tagPage.storylinesContent && | ||
| // isSCTagPage && |
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.
⚡
| article.image?.isAvatar | ||
| ? article.image?.src | ||
| : undefined, // will need to be set for opinion pieces | ||
| // mainMedia: undefined, // ought to be set for multimedia pieces, but missing the extra info like count? |
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.
Is this a TBD?
| // const articleAge = | ||
| // article.publicationTime && | ||
| // timeAgo(new Date(article.publicationTime).getTime()).toString(); |
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.
⚡
| categories: ParsedCategory[]; | ||
| }; | ||
|
|
||
| // probably want to add a generic category type mapping to those in supercharger (e.g. opinions) and map this to a container type and title (e.g. "Contrasting Opinions" + "flexible/general") |
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.
💯
| <> | ||
| <StorylineSection | ||
| title="Storylines" | ||
| containerPalette="LongRunningAltPalette" |
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.
Feels a bit hacky to hard code this, but this provides some styling (notably the greyed background) without having to add more palette variations and still sticking to the “container overrides” pattern in a section.
155c448 to
3c40535
Compare
What does this change?
Part of the AI tag pages work. This adds the content to the tag pages model, enabling it to be passed through from the work in this frontend PR, implements the figma designs (available here although there’s also been some subsequent decisions not updated in the figma), and includes new storybook stories for testing.
Worth flagging at the outset that this is all behind a 0% experiment in frontend - if not in the test, frontend doesn't retrieve the needed content (just returning None). In January, we plan to start a 50% test on a limited number of pages; after which we'll determine if we want to keep the feature and deploy it on a wider number of tags.
It looks like a big PR, but it's meant to comprehensively deliver the feature (with the associated model, data transformation and rendering changes, which has also meant splitting this up is arguably not ideal). A significant portion of the diff comes from code which is either very similar to an existing component, or from schema changes/fixtures/storybook setup.
Chromatic tests only flagged one false positive for an existing component (change of the classname), so I'm fairly confident this doesn't affect existing styling.
What does this actually change?
We add the content type to the tag page model, based on the structure provided by the tool and correspondingly sent by frontend.
As a quick overview, we have:
-> content
---> array of storylines
----> in each storyline, an array of categories (e.g. key stories, opinions, deep reads)
-----> in each category, an array of articles, with the minimum data required to render them by DCR (headline, url, publication date, maybe byline, image data and if relevant, main media data).
The content gets rendered in the tag page layout just below the first container (thereby always showing the most recent articles on a given tag at the top of the page). The designs are inspired by the front page redesign, and notably the flexible general container, so the implementation has taken this into account and tried to reuse existing logic where possible.
We add two new components,
StorylineSectionandStorylinesSection.importable. The former provides the general structure of the section (it's very similar to theFrontSectioncomponent, particularly in terms of the style and grid setup, but slightly adapted and removing unnecessary logic).The latter calls an
enhanceAITagPageContentfunction to transform the data sent by frontend, turning the very basic article information we receive (headline, url, image/multimedia data) into theDCRGroupedTrailsandDCRFrontCardshape. Based on the categories these articles are a part of, we can be deterministic in terms of how values need to be defined for aDCRFrontCard(e.g. for the format). Most of these front cards end up being part of the standard group in a trail, but the key stories category is a bit different: the image for that gets taken from the first (latest) article in the category, and each of the headlines get used as the "supporting content" on the left of the image.Once we've got the data in shape, each of the categories in a storyline gets rendered as a flexible general container. These are individual containers mainly so we can add the title of the category before them.
For the most part this just uses the pre-existing logic, but some variations needed to be tweaked (e.g. the rendering of the key stories category in particular), and so we pass a boolean
storylinesStylethroughFlexibleGeneralandCardto enable this (and add aSupportingKeyStoriesContentcomponent, similar to the supporting content component, but with enough notable differences).CardAgeand feature card components (FeatureCard andFeatureCardAge`) also get touched slightly, to ensure we display the age correctly (as the cards are displayed on an “archive” page, it feels important to indicate when the article was published).The only piece of AI generated text, the titles of the storylines, get rendered at the top of the section. These are tabs that can be clicked on to switch stories, so we need this to be an island to enable the necessary javascript.
Few more points to note:
Screenshots
[keyStories-mobile]:
[opinionsAndExplainers-mobile]:
[deepReads-mobile]:
[multimedia-mobile]:
[profilesAndInterviews-mobile]: