From 95ed6f78b0f72d07fe65d48d7417b71bca95d76e Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Wed, 4 Oct 2023 15:11:30 +0100 Subject: [PATCH 01/19] refactor branding --- .../src/components/FrontSection.stories.tsx | 52 +++-- .../src/components/FrontSection.tsx | 147 +------------- .../src/components/FrontSectionTitle.tsx | 145 ++++++++++++++ dotcom-rendering/src/layouts/FrontLayout.tsx | 37 ++-- .../src/lib/assert-unreachable.ts | 12 ++ dotcom-rendering/src/lib/branding.ts | 183 +++++++++++++++++- dotcom-rendering/src/lib/non-empty.test.ts | 7 + dotcom-rendering/src/lib/non-empty.ts | 10 + .../src/model/decideBadge.test.ts | 117 ----------- dotcom-rendering/src/model/decideBadge.ts | 63 ------ .../src/model/decideEditorialBadge.test.ts | 41 ++++ .../src/model/decideEditorialBadge.ts | 39 ++++ .../decideSponsoredContentBranding.test.ts | 120 ------------ .../model/decideSponsoredContentBranding.ts | 33 ---- .../src/model/enhanceCollections.ts | 58 ++---- .../src/server/index.front.web.ts | 2 +- dotcom-rendering/src/types/branding.ts | 19 ++ dotcom-rendering/src/types/front.ts | 7 +- 18 files changed, 522 insertions(+), 570 deletions(-) create mode 100644 dotcom-rendering/src/components/FrontSectionTitle.tsx create mode 100644 dotcom-rendering/src/lib/assert-unreachable.ts create mode 100644 dotcom-rendering/src/lib/non-empty.test.ts create mode 100644 dotcom-rendering/src/lib/non-empty.ts delete mode 100644 dotcom-rendering/src/model/decideBadge.test.ts delete mode 100644 dotcom-rendering/src/model/decideBadge.ts create mode 100644 dotcom-rendering/src/model/decideEditorialBadge.test.ts create mode 100644 dotcom-rendering/src/model/decideEditorialBadge.ts delete mode 100644 dotcom-rendering/src/model/decideSponsoredContentBranding.test.ts delete mode 100644 dotcom-rendering/src/model/decideSponsoredContentBranding.ts diff --git a/dotcom-rendering/src/components/FrontSection.stories.tsx b/dotcom-rendering/src/components/FrontSection.stories.tsx index 7784e1ec7c7..06f42213c66 100644 --- a/dotcom-rendering/src/components/FrontSection.stories.tsx +++ b/dotcom-rendering/src/components/FrontSection.stories.tsx @@ -246,46 +246,42 @@ TreatsStory.storyName = 'with treats and date header'; * Note only the first container should show a badge */ export const MultipleOnAPaidFront = () => { - const frontBranding = { - edition: { - id: 'UK', - }, - branding: { - brandingType: { - name: 'paid-content', - }, - sponsorName: 'Grounded', - logo: { - src: 'https://static.theguardian.com/commercial/sponsor/28/Oct/2020/daa941da-14fd-46cc-85cb-731ce59050ee-Grounded_badging-280x180.png', - dimensions: { - width: 140, - height: 90, - }, - link: '/', - label: 'Paid for by', - }, - aboutThisLink: - 'https://www.theguardian.com/info/2016/jan/25/content-funding', - }, - } as const; + // const frontBranding = { + // edition: { + // id: 'UK', + // }, + // branding: { + // brandingType: { + // name: 'paid-content', + // }, + // sponsorName: 'Grounded', + // logo: { + // src: 'https://static.theguardian.com/commercial/sponsor/28/Oct/2020/daa941da-14fd-46cc-85cb-731ce59050ee-Grounded_badging-280x180.png', + // dimensions: { + // width: 140, + // height: 90, + // }, + // link: '/', + // label: 'Paid for by', + // }, + // aboutThisLink: + // 'https://www.theguardian.com/info/2016/jan/25/content-funding', + // }, + // } as const; return ( <> diff --git a/dotcom-rendering/src/components/FrontSection.tsx b/dotcom-rendering/src/components/FrontSection.tsx index f46efa80b07..d38fcca2b4c 100644 --- a/dotcom-rendering/src/components/FrontSection.tsx +++ b/dotcom-rendering/src/components/FrontSection.tsx @@ -5,25 +5,21 @@ import { between, from, neutral, - palette, space, - textSans, until, } from '@guardian/source-foundations'; -import { Hide } from '@guardian/source-react-components'; import { pageSkinContainer } from '../layouts/lib/pageSkin'; import { decideContainerOverrides } from '../lib/decideContainerOverrides'; import type { EditionId } from '../lib/edition'; import { hideAge } from '../lib/hideAge'; -import type { DCRBadgeType } from '../types/badge'; -import type { Branding, EditionBranding } from '../types/branding'; +import type { CollectionBranding } from '../types/branding'; import type { DCRContainerPalette, TreatType } from '../types/front'; import type { DCRFrontPagination } from '../types/tagFront'; import { isAustralianTerritory, type Territory } from '../types/territory'; import { AustralianTerritorySwitcher } from './AustralianTerritorySwitcher.importable'; -import { Badge } from './Badge'; import { ContainerTitle } from './ContainerTitle'; import { FrontPagination } from './FrontPagination'; +import { FrontSectionTitle } from './FrontSectionTitle'; import { Island } from './Island'; import { ShowHideButton } from './ShowHideButton'; import { ShowMore } from './ShowMore.importable'; @@ -70,7 +66,6 @@ type Props = { editionId?: EditionId; /** A list of related links that appear in the bottom of the left column on fronts */ treats?: TreatType[]; - badge?: DCRBadgeType; /** Enable the "Show More" button on this container to allow readers to load more cards */ canShowMore?: boolean; ajaxUrl?: string; @@ -79,8 +74,6 @@ type Props = { pagination?: DCRFrontPagination; /** Does this front section reside on a "paid for" content front */ isOnPaidContentFront?: boolean; - /** Denotes the position of this section on the front */ - index?: number; /** Indicates if the container is targetted to a specific territory */ targetedTerritory?: Territory; /** Indicates if the page has a page skin advert @@ -92,8 +85,7 @@ type Props = { */ hasPageSkin?: boolean; discussionApiUrl: string; - frontBranding?: EditionBranding; - containerBranding?: Branding; + collectionBranding?: CollectionBranding; }; const width = (columns: number, columnWidth: number, columnGap: number) => @@ -347,12 +339,6 @@ const bottomPadding = css` padding-bottom: ${space[9]}px; `; -const titleStyle = css` - ${until.leftCol} { - max-width: 74%; - } -`; - const decideBackgroundColour = ( overrideBackgroundColour: string | undefined, hasPageSkin: boolean, @@ -366,44 +352,6 @@ const decideBackgroundColour = ( return undefined; }; -const labelStyles = css` - ${textSans.xxsmall()}; - line-height: 1rem; - color: ${palette.neutral[46]}; - font-weight: bold; - margin-top: 0.375rem; - padding-right: 0.625rem; - padding-bottom: 0.625rem; - text-align: left; -`; - -const aboutThisLinkStyles = css` - ${textSans.xxsmall()}; - line-height: 11px; - color: ${palette.neutral[46]}; - font-weight: normal; - text-decoration: none; -`; - -const SponsoredBranding = ({ - branding, -}: { - branding: Branding | undefined; -}) => { - if (!branding) { - return null; - } - const { logo, aboutThisLink } = branding; - return ( - <> -

{logo.label}

- - - About this content - - - ); -}; /** * # Front Container * @@ -504,17 +452,14 @@ export const FrontSection = ({ toggleable = false, treats, url, - badge, canShowMore, ajaxUrl, pagination, isOnPaidContentFront, - index, targetedTerritory, hasPageSkin = false, - frontBranding, discussionApiUrl, - containerBranding, + collectionBranding, }: Props) => { const overrides = containerPalette && decideContainerOverrides(containerPalette); @@ -528,21 +473,6 @@ export const FrontSection = ({ !!pageId && !!ajaxUrl; - const frontHasEditorialOrDesignBadge = !!badge; - - // Only show the badge with a "Paid for by" label on the FIRST card of a paid front, aka as Labs front. - const isTheFirstContainerOnAPaidFront = - isOnPaidContentFront && index === 0 && !!frontBranding; - - const isTheFirstContainerOnASponsoredFront = - !isOnPaidContentFront && index === 0 && !!frontBranding; - - const showSponsoredBranding = - (isTheFirstContainerOnASponsoredFront && frontBranding.branding) || - containerBranding; - - const sponsoredBranding = frontBranding?.branding ?? containerBranding; - /** * id is being used to set the containerId in @see {ShowMore.importable.tsx} * this id pre-existed showMore so is probably also being used for something else. @@ -590,78 +520,21 @@ export const FrontSection = ({ sectionHeadlineHeight, ]} > - {isTheFirstContainerOnAPaidFront ? ( -
+ - {!!frontBranding.branding?.logo.src && ( -
- Paid for by - -
- )} -
- ) : ( - <> - {frontHasEditorialOrDesignBadge && ( - - - - )} -
- {frontHasEditorialOrDesignBadge && ( - - - - )} - - {showSponsoredBranding && ( - - )} -
- - )} + } + collectionBranding={collectionBranding} + /> {leftContent} diff --git a/dotcom-rendering/src/components/FrontSectionTitle.tsx b/dotcom-rendering/src/components/FrontSectionTitle.tsx new file mode 100644 index 00000000000..4ee9c42e5ee --- /dev/null +++ b/dotcom-rendering/src/components/FrontSectionTitle.tsx @@ -0,0 +1,145 @@ +import { css } from '@emotion/react'; +import { from, palette, textSans, until } from '@guardian/source-foundations'; +import { Hide } from '@guardian/source-react-components'; +import { assertUnreachable } from '../lib/assert-unreachable'; +import type { CollectionBranding } from '../types/branding'; +import { Badge } from './Badge'; + +type Props = { + title: React.ReactNode; + collectionBranding: CollectionBranding | undefined; +}; + +const titleStyle = css` + ${until.leftCol} { + max-width: 74%; + } +`; + +const labelStyles = css` + ${textSans.xxsmall()}; + line-height: 1rem; + color: ${palette.neutral[46]}; + font-weight: bold; + margin-top: 0.375rem; + padding-right: 0.625rem; + padding-bottom: 0.625rem; + text-align: left; +`; + +const aboutThisLinkStyles = css` + ${textSans.xxsmall()}; + line-height: 11px; + color: ${palette.neutral[46]}; + font-weight: normal; + text-decoration: none; +`; + +export const FrontSectionTitle = ({ title, collectionBranding }: Props) => { + switch (collectionBranding?.kind) { + case 'editorial': { + const { + badge: { imageSrc, href }, + } = collectionBranding; + return ( + <> + + + +
+ + + + {title} +
+ + ); + } + case 'foundation': { + const { + branding: { logo }, + } = collectionBranding; + return ( + <> + + + +
+ + + + {title} +
+ + ); + } + case 'paid-content': { + const { + isFrontBranding, + branding: { logo }, + } = collectionBranding; + + if (isFrontBranding) { + return ( +
+ {title} +
+ Paid for by + +
+
+ ); + } + + return ( + <> + + + +
+ + + + {title} +
+ + ); + } + case 'sponsored': { + const { + branding: { logo, aboutThisLink }, + } = collectionBranding; + return ( +
+ {title} + <> +

{logo.label}

+ + + About this content + + +
+ ); + } + case undefined: { + return
{title}
; + } + default: { + assertUnreachable(collectionBranding); + return null; + } + } +}; diff --git a/dotcom-rendering/src/layouts/FrontLayout.tsx b/dotcom-rendering/src/layouts/FrontLayout.tsx index e746f4f7322..4342e7afe58 100644 --- a/dotcom-rendering/src/layouts/FrontLayout.tsx +++ b/dotcom-rendering/src/layouts/FrontLayout.tsx @@ -32,6 +32,7 @@ import { StickyBottomBanner } from '../components/StickyBottomBanner.importable' import { SubNav } from '../components/SubNav.importable'; import { TrendingTopics } from '../components/TrendingTopics'; import { WeatherWrapper } from '../components/WeatherWrapper.importable'; +import { badgeFromBranding } from '../lib/branding'; import { canRenderAds } from '../lib/canRenderAds'; import { getContributionsServiceUrl } from '../lib/contributions'; import { decideContainerOverrides } from '../lib/decideContainerOverrides'; @@ -319,21 +320,16 @@ export const FrontLayout = ({ front, NAV }: Props) => { } | ${ophanName}`; const mostPopularTitle = 'Most popular'; - const trailsWithoutBranding = collection.paidContentBadge - ? trails.map((labTrail) => { - return { - ...labTrail, - branding: undefined, - }; - }) - : trails; - - const frontEditionBranding = - front.pressedPage.frontProperties.commercial.editionBrandings.find( - (eB) => - eB.edition.id === front.editionId && - !!eB.branding, - ); + // Remove the branding from each of the cards in a paid content collection + const trailsWithoutBranding = + collection.collectionBranding?.kind === 'paid-content' + ? trails.map((labTrail) => { + return { + ...labTrail, + branding: undefined, + }; + }) + : trails; if (collection.collectionType === 'fixed/thrasher') { return ( @@ -499,7 +495,9 @@ export const FrontLayout = ({ front, NAV }: Props) => { containerName={collection.collectionType} canShowMore={collection.canShowMore} url={collection.href} - badge={collection.paidContentBadge} + badge={badgeFromBranding( + collection.collectionBranding, + )} data-print-layout="hide" hasPageSkin={hasPageSkin} discussionApiUrl={ @@ -655,7 +653,6 @@ export const FrontLayout = ({ front, NAV }: Props) => { collection, hasPageSkin, )} - badge={collection.editorialBadge} sectionId={ophanName} collectionId={collection.id} pageId={front.pressedPage.id} @@ -667,13 +664,11 @@ export const FrontLayout = ({ front, NAV }: Props) => { canShowMore={collection.canShowMore} ajaxUrl={front.config.ajaxUrl} isOnPaidContentFront={isPaidContent} - index={index} targetedTerritory={collection.targetedTerritory} hasPageSkin={hasPageSkin} - frontBranding={frontEditionBranding} discussionApiUrl={front.config.discussionApiUrl} - containerBranding={ - collection.sponsoredContentBranding + collectionBranding={ + collection.collectionBranding } > { + throw new Error('This should be unreachable'); +}; diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index b7cbcb0a294..3be636dca49 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -1,5 +1,17 @@ -import type { Branding, EditionBranding } from '../types/branding'; +import { decideEditorialBadge } from '../model/decideEditorialBadge'; +import type { DCRBadgeType } from '../types/badge'; +import type { + Branding, + BrandingKind, + CollectionBranding, + EditionBranding, +} from '../types/branding'; +import type { FEFrontCard } from '../types/front'; +import { assertUnreachable } from './assert-unreachable'; import type { EditionId } from './edition'; +import { guard } from './guard'; +import type { NonEmpty } from './non-empty'; +import { isNonEmpty } from './non-empty'; export const pickBrandingForEdition = ( editionBrandings: EditionBranding[], @@ -8,3 +20,172 @@ export const pickBrandingForEdition = ( editionBrandings.find( ({ edition, branding }) => edition.id === editionId && branding, )?.branding; + +const getBrandingFromCards = ( + cards: FEFrontCard[], + editionId: EditionId, +): NonEmpty | undefined => { + const brandings: Branding[] = []; + for (const card of cards) { + const branding = pickBrandingForEdition( + card.properties.editionBrandings, + editionId, + ); + // If a single card is missing branding then we bail out immediately + if (!branding) { + return undefined; + } + brandings.push(branding); + } + if (!isNonEmpty(brandings)) { + return undefined; + } + return brandings; +}; + +const isCollectionBrandingKind = guard([ + 'paid-content', + 'sponsored', + 'foundation', +] as const); + +const getBrandingType = ([firstBranding, ...restBranding]: NonEmpty): + | BrandingKind + | undefined => { + const name = firstBranding.brandingType?.name; + + if (!isCollectionBrandingKind(name)) { + return undefined; + } + + const allNamesMatch = restBranding.every( + ({ brandingType }) => brandingType?.name === name, + ); + + if (!allNamesMatch) { + return undefined; + } + + return name; +}; + +const everyCardHasSameSponsor = ([ + firstBranding, + ...restBranding +]: NonEmpty): boolean => + restBranding.every( + (branding) => branding.sponsorName === firstBranding.sponsorName, + ); + +/** + * TODO verify that this is what is necessary for two branding to be equal + */ +const brandingEqual = (b1: Branding, b2: Branding) => { + return ( + b1.sponsorName === b2.sponsorName && + b1.brandingType?.name === b2.brandingType?.name + ); +}; + +export const badgeFromBranding = ( + collectionBranding: CollectionBranding | undefined, +): DCRBadgeType | undefined => { + switch (collectionBranding?.kind) { + case 'paid-content': + case 'sponsored': + case 'foundation': { + const { logo } = collectionBranding.branding; + return { + imageSrc: logo.src, + href: logo.link, + }; + } + case 'editorial': { + return collectionBranding.badge; + } + case undefined: { + return undefined; + } + } +}; + +export const decideCollectionBranding = ({ + frontBranding, + index, + seriesTag, + cards, + editionId, +}: { + frontBranding: Branding | undefined; + index: number; + seriesTag: string | undefined; + cards: FEFrontCard[]; + editionId: EditionId; +}): CollectionBranding | undefined => { + // TODO it might not be enough to first check the first index + // For example, what if the first container is a thrasher? + if (index === 0 && frontBranding !== undefined) { + const kind = getBrandingType([frontBranding]); + if (!kind) { + return undefined; + } + return { + kind, + isFrontBranding: true, + branding: frontBranding, + }; + } + + const editorialBadge = decideEditorialBadge(seriesTag); + if (editorialBadge) { + return { + kind: 'editorial', + badge: editorialBadge, + }; + } + + const brandingForCards = getBrandingFromCards(cards, editionId); + + if (!brandingForCards) { + return undefined; + } + + const kind = getBrandingType(brandingForCards); + + if (!kind) { + return undefined; + } + + const [branding] = brandingForCards; + + if (frontBranding && brandingEqual(frontBranding, branding)) { + return undefined; + } + + switch (kind) { + case 'foundation': + case 'paid-content': { + return { + kind, + isFrontBranding: false, + branding, + }; + } + case 'sponsored': { + // We do an additional check on sponsored branding + // Ensuring that each has the same sponsor name + if (!everyCardHasSameSponsor(brandingForCards)) { + return undefined; + } + return { + kind, + isFrontBranding: false, + branding, + }; + } + default: { + return assertUnreachable(kind); + } + } +}; +export { assertUnreachable }; diff --git a/dotcom-rendering/src/lib/non-empty.test.ts b/dotcom-rendering/src/lib/non-empty.test.ts new file mode 100644 index 00000000000..38f32e0b7d2 --- /dev/null +++ b/dotcom-rendering/src/lib/non-empty.test.ts @@ -0,0 +1,7 @@ +import { isNonEmpty } from './non-empty'; + +it('isNonEmpty', () => { + expect(isNonEmpty([])).toBe(false); + expect(isNonEmpty([1])).toBe(true); + expect(isNonEmpty([1, 2, 3])).toBe(true); +}); diff --git a/dotcom-rendering/src/lib/non-empty.ts b/dotcom-rendering/src/lib/non-empty.ts new file mode 100644 index 00000000000..b1f32fada2f --- /dev/null +++ b/dotcom-rendering/src/lib/non-empty.ts @@ -0,0 +1,10 @@ +/** + * TODO + */ +export type NonEmpty = [T, ...T[]]; + +/** + * TODO + */ +export const isNonEmpty = (items: T[]): items is NonEmpty => + items.length > 0; diff --git a/dotcom-rendering/src/model/decideBadge.test.ts b/dotcom-rendering/src/model/decideBadge.test.ts deleted file mode 100644 index 5d7de341863..00000000000 --- a/dotcom-rendering/src/model/decideBadge.test.ts +++ /dev/null @@ -1,117 +0,0 @@ -import { decideEditorialBadge, decidePaidContentBadge } from './decideBadge'; - -jest.mock('./badges'); - -const brandingAmazon = { - brandingType: { - name: 'paid-content', - }, - sponsorName: 'Amazon', - logo: { - src: 'https://static.theguardian.com/commercial/sponsor/04/Oct/2018/6b15ba78-da66-415d-8540-a34cc4d3156b-romanoffs_TT_PO-center.png', - dimensions: { - width: 140, - height: 90, - }, - link: 'https://www.amazon.com/dp/B07FV6K8HF', - label: 'Paid for by', - }, - aboutThisLink: - 'https://www.theguardian.com/info/2016/jan/25/content-funding', -} as const; - -const brandingGuardianOrg = { - brandingType: { - name: 'sponsored', - }, - sponsorName: 'guardian.org', - logo: { - src: 'https://static.theguardian.com/commercial/sponsor/16/Mar/2022/1e17c6f8-8114-44e9-8e10-02b8e5c6b929-theguardianorg badge.png', - dimensions: { - width: 280, - height: 180, - }, - link: 'https://www.theguardian.com/global-development/2021/feb/21/about-the-rights-and-freedom-series', - label: 'Supported by', - }, - logoForDarkBackground: { - src: 'https://static.theguardian.com/commercial/sponsor/16/Mar/2022/2cb64c63-e09c-4877-90ee-b5fe4eac7fc6-44d00539-51bd-4749-a16a-7dde8ff8e19b-a060074a-c6d4-4e6e-b33a-8f4930d5617c-g.org_hc.png', - dimensions: { - width: 280, - height: 180, - }, - link: 'https://www.theguardian.com/global-development/2021/feb/21/about-the-rights-and-freedom-series', - label: 'Supported by', - }, - aboutThisLink: - 'https://www.theguardian.com/global-development/2021/feb/21/about-the-rights-and-freedom-series', -} as const; - -describe('Decide badge', () => { - describe('getBadgeFromSeriesTag', () => { - it('returns correct standard badge', () => { - const tagId = 'uk-news/series/the-brexit-gamble'; - const expectedResult = { - href: `/${tagId}`, - imageSrc: `/static/frontend/badges/EUReferendumBadge.svg`, - }; - const result = decideEditorialBadge(tagId); - expect(result).toMatchObject(expectedResult); - }); - - it('returns correct special badge', () => { - const tagId = 'tone/newsletter-tone'; - const expectedResult = { - href: `/${tagId}`, - imageSrc: `/static/frontend/badges/newsletter-badge.svg`, - }; - const result = decideEditorialBadge(tagId); - expect(result).toMatchObject(expectedResult); - }); - - it('returns undefined if no standard or special badge match found for series tag', () => { - const tagId = 'lifeandstyle/home-and-garden'; - const expectedResult = undefined; - const result = decideEditorialBadge(tagId); - expect(result).toEqual(expectedResult); - }); - - it('returns undefined for undefined series tag', () => { - const tagId = undefined; - const expectedResult = undefined; - const result = decideEditorialBadge(tagId); - expect(result).toEqual(expectedResult); - }); - }); - - describe('getBadgeFromBranding function', () => { - it('returns properties of the first badge if all cards have the same sponsor', () => { - const branding = [brandingAmazon, brandingAmazon]; - - const expectedResult = { - imageSrc: brandingAmazon.logo.src, - href: brandingAmazon.logo.link, - }; - const result = decidePaidContentBadge(branding); - expect(result).toEqual(expectedResult); - }); - - it('returns undefined if all cards do not have the same sponsor', () => { - const branding = [brandingAmazon, brandingGuardianOrg]; - - const expectedResult = undefined; - const result = decidePaidContentBadge(branding); - expect(result).toEqual(expectedResult); - }); - - it('returns undefined if no branding supplied', () => { - const expectedResult = undefined; - - const result = decidePaidContentBadge(undefined); - expect(result).toEqual(expectedResult); - - const result2 = decidePaidContentBadge([]); - expect(result2).toEqual(expectedResult); - }); - }); -}); diff --git a/dotcom-rendering/src/model/decideBadge.ts b/dotcom-rendering/src/model/decideBadge.ts deleted file mode 100644 index e82eec222a3..00000000000 --- a/dotcom-rendering/src/model/decideBadge.ts +++ /dev/null @@ -1,63 +0,0 @@ -import { createHash } from 'node:crypto'; -import { ASSET_ORIGIN } from '../lib/assets'; -import type { DCRBadgeType } from '../types/badge'; -import type { Branding } from '../types/branding'; -import { BADGES, SPECIAL_BADGES } from './badges'; - -/** - * Fetches the badge properties only if ALL branding has the same sponsor. - */ -export const decidePaidContentBadge = ( - branding?: Branding[], -): DCRBadgeType | undefined => { - // Early return if there are no branding elements - if (!branding) return; - if (!branding.length) return; - - const [firstBrand] = branding; - // Early return if the first brand is falsy - if (!firstBrand) return; - - const allBrandingHasSameSponsor = branding.every( - ({ sponsorName }) => sponsorName === firstBrand.sponsorName, - ); - - return allBrandingHasSameSponsor - ? { - imageSrc: firstBrand.logo.src, - href: firstBrand.logo.link, - } - : undefined; -}; - -/** - * Fetches the corresponding badge using the series tag, if there's a match in the lookup. - */ -export const decideEditorialBadge = ( - seriesTag?: string, -): DCRBadgeType | undefined => { - if (!seriesTag) return undefined; - - const badge = BADGES.find((b) => b.seriesTag === seriesTag); - if (badge) { - return { - imageSrc: `${ASSET_ORIGIN}static/frontend/${badge.imageSrc}`, - href: `/${seriesTag}`, - }; - } else { - // "Special" hidden badges have their series tags hashed - const specialBadge = SPECIAL_BADGES.find((b) => { - const badgeHash = createHash('md5') - .update(b.salt + seriesTag) - .digest('hex'); - return badgeHash.includes(b.hashedTag); - }); - - return specialBadge - ? { - imageSrc: `${ASSET_ORIGIN}static/frontend/${specialBadge.imageSrc}`, - href: `/${seriesTag}`, - } - : undefined; // No badge or special badge found - } -}; diff --git a/dotcom-rendering/src/model/decideEditorialBadge.test.ts b/dotcom-rendering/src/model/decideEditorialBadge.test.ts new file mode 100644 index 00000000000..48f7c5fc5d9 --- /dev/null +++ b/dotcom-rendering/src/model/decideEditorialBadge.test.ts @@ -0,0 +1,41 @@ +import { decideEditorialBadge } from './decideEditorialBadge'; + +jest.mock('./badges'); + +describe('Decide badge', () => { + describe('getBadgeFromSeriesTag', () => { + it('returns correct standard badge', () => { + const tagId = 'uk-news/series/the-brexit-gamble'; + const expectedResult = { + href: `/${tagId}`, + imageSrc: `/static/frontend/badges/EUReferendumBadge.svg`, + }; + const result = decideEditorialBadge(tagId); + expect(result).toMatchObject(expectedResult); + }); + + it('returns correct special badge', () => { + const tagId = 'tone/newsletter-tone'; + const expectedResult = { + href: `/${tagId}`, + imageSrc: `/static/frontend/badges/newsletter-badge.svg`, + }; + const result = decideEditorialBadge(tagId); + expect(result).toMatchObject(expectedResult); + }); + + it('returns undefined if no standard or special badge match found for series tag', () => { + const tagId = 'lifeandstyle/home-and-garden'; + const expectedResult = undefined; + const result = decideEditorialBadge(tagId); + expect(result).toEqual(expectedResult); + }); + + it('returns undefined for undefined series tag', () => { + const tagId = undefined; + const expectedResult = undefined; + const result = decideEditorialBadge(tagId); + expect(result).toEqual(expectedResult); + }); + }); +}); diff --git a/dotcom-rendering/src/model/decideEditorialBadge.ts b/dotcom-rendering/src/model/decideEditorialBadge.ts new file mode 100644 index 00000000000..f6cdee0dee6 --- /dev/null +++ b/dotcom-rendering/src/model/decideEditorialBadge.ts @@ -0,0 +1,39 @@ +import { createHash } from 'node:crypto'; +import { ASSET_ORIGIN } from '../lib/assets'; +import type { DCRBadgeType } from '../types/badge'; +import { BADGES, SPECIAL_BADGES } from './badges'; + +/** + * Fetches the corresponding badge using the series tag, if there's a match in the lookup. + */ +export const decideEditorialBadge = ( + seriesTag?: string, +): DCRBadgeType | undefined => { + if (!seriesTag) return undefined; + + const badge = BADGES.find((b) => b.seriesTag === seriesTag); + if (badge) { + return { + imageSrc: `${ASSET_ORIGIN}static/frontend/${badge.imageSrc}`, + href: `/${seriesTag}`, + }; + } + + // "Special" hidden badges have their series tags hashed + const specialBadge = SPECIAL_BADGES.find((b) => { + const badgeHash = createHash('md5') + .update(b.salt + seriesTag) + .digest('hex'); + return badgeHash.includes(b.hashedTag); + }); + + if (specialBadge) { + return { + imageSrc: `${ASSET_ORIGIN}static/frontend/${specialBadge.imageSrc}`, + href: `/${seriesTag}`, + }; + } + + // No badge or special badge found + return undefined; +}; diff --git a/dotcom-rendering/src/model/decideSponsoredContentBranding.test.ts b/dotcom-rendering/src/model/decideSponsoredContentBranding.test.ts deleted file mode 100644 index 843740a29f3..00000000000 --- a/dotcom-rendering/src/model/decideSponsoredContentBranding.test.ts +++ /dev/null @@ -1,120 +0,0 @@ -import type { Branding } from '../types/branding'; -import { decideSponsoredContentBranding } from './decideSponsoredContentBranding'; - -const editionBrandingWithSponsor: Branding = { - brandingType: { - name: 'sponsored', - }, - sponsorName: 'guardian.org', - logo: { - src: 'https://static.theguardian.com/commercial/sponsor/16/Mar/2022/1e17c6f8-8114-44e9-8e10-02b8e5c6b929-theguardianorg badge.png', - dimensions: { - width: 280, - height: 180, - }, - link: 'https://www.theguardian.com/global-development/2021/feb/21/about-the-rights-and-freedom-series', - label: 'Supported by', - }, - logoForDarkBackground: { - src: 'https://static.theguardian.com/commercial/sponsor/16/Mar/2022/2cb64c63-e09c-4877-90ee-b5fe4eac7fc6-44d00539-51bd-4749-a16a-7dde8ff8e19b-a060074a-c6d4-4e6e-b33a-8f4930d5617c-g.org_hc.png', - dimensions: { - width: 280, - height: 180, - }, - link: 'https://www.theguardian.com/global-development/2021/feb/21/about-the-rights-and-freedom-series', - label: 'Supported by', - }, - aboutThisLink: - 'https://www.theguardian.com/global-development/2021/feb/21/about-the-rights-and-freedom-series', -}; - -describe('decideSponsoredContentBranding', () => { - it('returns sponsored branding if all cards in a collection have sponsored branding, they have the same sponsor and front has the same branding', () => { - expect( - decideSponsoredContentBranding( - 4, - [ - editionBrandingWithSponsor, - editionBrandingWithSponsor, - editionBrandingWithSponsor, - editionBrandingWithSponsor, - ], - true, - 'fixed/small/slow-IV', - ), - ).toEqual(editionBrandingWithSponsor); - }); - - it('returns undefined if at least one card in the collection does not have sponsored branding', () => { - expect( - decideSponsoredContentBranding( - 4, - Array(3).fill(editionBrandingWithSponsor), - false, - 'fixed/small/slow-IV', - ), - ).toEqual(undefined); - }); - - it('returns undefined if at least one card in the collection has different sponsor from the rest', () => { - const editionBrandingWithDifferentSponsor: Branding = { - brandingType: { - name: 'sponsored', - }, - sponsorName: 'Bertha Foundation', - logo: { - src: 'https://static.theguardian.com/commercial/sponsor/28/Mar/2017/d1105d96-b067-4091-81ce-02f7c7c24173-Logo.png', - dimensions: { - width: 108, - height: 45, - }, - link: 'http://www.berthafoundation.org/', - label: 'Supported by', - }, - logoForDarkBackground: { - src: 'https://static.theguardian.com/commercial/sponsor/28/Mar/2017/8aabe16e-fd35-4f3c-a39b-3ec149877f56-Logo.png', - dimensions: { - width: 108, - height: 45, - }, - link: 'http://www.berthafoundation.org/', - label: 'Supported by', - }, - aboutThisLink: - 'https://www.theguardian.com/info/2016/aug/31/about-guardian-bertha-documentary-partnership', - }; - - expect( - decideSponsoredContentBranding( - 4, - Array(3) - .fill(editionBrandingWithSponsor) - .fill(editionBrandingWithDifferentSponsor), - false, - 'fixed/small/slow-IV', - ), - ).toEqual(undefined); - }); - - it('returns undefined if front does not have the same branding as the cards', () => { - expect( - decideSponsoredContentBranding( - 4, - Array(4).fill(editionBrandingWithSponsor), - false, - 'fixed/small/slow-IV', - ), - ).toEqual(undefined); - }); - - it('returns undefined if the container is a thrasher', () => { - expect( - decideSponsoredContentBranding( - 4, - Array(4).fill(editionBrandingWithSponsor), - true, - 'fixed/thrasher', - ), - ).toEqual(undefined); - }); -}); diff --git a/dotcom-rendering/src/model/decideSponsoredContentBranding.ts b/dotcom-rendering/src/model/decideSponsoredContentBranding.ts deleted file mode 100644 index af9488a9617..00000000000 --- a/dotcom-rendering/src/model/decideSponsoredContentBranding.ts +++ /dev/null @@ -1,33 +0,0 @@ -import type { Branding } from '../types/branding'; -import type { DCRContainerType } from '../types/front'; - -export const decideSponsoredContentBranding = ( - collectionsLength: number, - allCardsBranding: Branding[], - editionHasBranding: boolean, - collectionType: DCRContainerType, -): Branding | undefined => { - const allCardsHaveBranding = collectionsLength === allCardsBranding.length; - - const allCardsHaveSponsoredBranding = - allCardsHaveBranding && - allCardsBranding.every( - (branding) => branding.brandingType?.name === 'sponsored', - ); - const allCardsHaveTheSameSponsor = - allCardsHaveBranding && - allCardsBranding.every( - (branding) => - branding.sponsorName === allCardsBranding[0]?.sponsorName, - ); - - const isNotAThrasher = collectionType !== 'fixed/thrasher'; - - const shouldHaveSponsorBranding = - isNotAThrasher && - editionHasBranding && - allCardsHaveSponsoredBranding && - allCardsHaveTheSameSponsor; - - return shouldHaveSponsorBranding ? allCardsBranding[0] : undefined; -}; diff --git a/dotcom-rendering/src/model/enhanceCollections.ts b/dotcom-rendering/src/model/enhanceCollections.ts index 2ac8af12f18..3793f2d0749 100644 --- a/dotcom-rendering/src/model/enhanceCollections.ts +++ b/dotcom-rendering/src/model/enhanceCollections.ts @@ -1,15 +1,8 @@ -import { isNonNullable } from '@guardian/libs'; -import { pickBrandingForEdition } from '../lib/branding'; +import { decideCollectionBranding } from '../lib/branding'; import type { EditionId } from '../lib/edition'; import type { Branding } from '../types/branding'; -import type { - DCRCollectionType, - FECollectionType, - FEFrontCard, -} from '../types/front'; -import { decideEditorialBadge, decidePaidContentBadge } from './decideBadge'; +import type { DCRCollectionType, FECollectionType } from '../types/front'; import { decideContainerPalette } from './decideContainerPalette'; -import { decideSponsoredContentBranding } from './decideSponsoredContentBranding'; import { enhanceCards } from './enhanceCards'; import { enhanceTreats } from './enhanceTreats'; import { groupCards } from './groupCards'; @@ -31,17 +24,6 @@ const isSupported = (collection: FECollectionType): boolean => ) ); -function getBrandingFromCards( - allCards: FEFrontCard[], - editionId: EditionId, -): Branding[] { - return allCards - .map((card) => - pickBrandingForEdition(card.properties.editionBrandings, editionId), - ) - .filter(isNonNullable); -} - export const enhanceCollections = ({ collections, editionId, @@ -49,7 +31,7 @@ export const enhanceCollections = ({ discussionApiUrl, frontBranding, onPageDescription, - isPaidContent, + isOnPaidContentFront, }: { collections: FECollectionType[]; editionId: EditionId; @@ -57,17 +39,19 @@ export const enhanceCollections = ({ discussionApiUrl: string; frontBranding: Branding | undefined; onPageDescription?: string; - isPaidContent?: boolean; + isOnPaidContentFront?: boolean; }): DCRCollectionType[] => { return collections.filter(isSupported).map((collection, index) => { const { id, displayName, collectionType, hasMore, href, description } = collection; const allCards = [...collection.curated, ...collection.backfill]; - const allBranding = getBrandingFromCards(allCards, editionId); - const allCardsHaveBranding = allCards.length === allBranding.length; - const isCollectionPaidContent = allBranding.every( - ({ brandingType }) => brandingType?.name === 'paid-content', - ); + const collectionBranding = decideCollectionBranding({ + frontBranding, + index, + seriesTag: collection.config.href, + cards: allCards, + editionId, + }); const containerPalette = decideContainerPalette( collection.config.metadata?.map((meta) => meta.type), @@ -77,9 +61,8 @@ export const enhanceCollections = ({ */ { canBeBranded: - !isPaidContent && - allCardsHaveBranding && - isCollectionPaidContent, + !isOnPaidContentFront && + collectionBranding?.kind === 'paid-content', }, ); @@ -93,20 +76,7 @@ export const enhanceCollections = ({ collectionType, href, containerPalette, - editorialBadge: decideEditorialBadge(collection.config.href), - paidContentBadge: decidePaidContentBadge( - // We only try to use a branded badge for paid content - isCollectionPaidContent && allCardsHaveBranding - ? allBranding - : undefined, - ), - sponsoredContentBranding: decideSponsoredContentBranding( - allCards.length, - allBranding, - // TODO(@chrislomaxjones) Read the full front branding value - !!frontBranding, - collectionType, - ), + collectionBranding, grouped: groupCards( collectionType, collection.curated, diff --git a/dotcom-rendering/src/server/index.front.web.ts b/dotcom-rendering/src/server/index.front.web.ts index 78170bc8f84..2b10ddd7231 100644 --- a/dotcom-rendering/src/server/index.front.web.ts +++ b/dotcom-rendering/src/server/index.front.web.ts @@ -33,7 +33,7 @@ const enhanceFront = (body: unknown): DCRFrontType => { pageId: data.pageId, onPageDescription: data.pressedPage.frontProperties.onPageDescription, - isPaidContent: data.config.isPaidContent, + isOnPaidContentFront: data.config.isPaidContent, discussionApiUrl: data.config.discussionApiUrl, frontBranding: pickBrandingForEdition( data.pressedPage.frontProperties.commercial diff --git a/dotcom-rendering/src/types/branding.ts b/dotcom-rendering/src/types/branding.ts index f575155fe7b..0f13d84d453 100644 --- a/dotcom-rendering/src/types/branding.ts +++ b/dotcom-rendering/src/types/branding.ts @@ -1,4 +1,5 @@ import type { EditionId } from '../lib/edition'; +import type { DCRBadgeType } from './badge'; type BrandingLogo = { src: string; @@ -29,3 +30,21 @@ export interface EditionBranding { }; branding?: Branding; } + +export type BrandingKind = 'paid-content' | 'foundation' | 'sponsored'; + +export type SponsorshipBranding = { + kind: BrandingKind; + isFrontBranding: boolean; + branding: Branding; +}; + +export type EditorialBranding = { + kind: 'editorial'; + badge: DCRBadgeType; +}; + +/** + * The type of branding that can be applied to any given collection + */ +export type CollectionBranding = SponsorshipBranding | EditorialBranding; diff --git a/dotcom-rendering/src/types/front.ts b/dotcom-rendering/src/types/front.ts index 8a1db93b031..aed24ef9a2d 100644 --- a/dotcom-rendering/src/types/front.ts +++ b/dotcom-rendering/src/types/front.ts @@ -1,8 +1,7 @@ import type { ArticleSpecial, Pillar } from '@guardian/libs'; import type { SharedAdTargeting } from '../lib/ad-targeting'; import type { EditionId } from '../lib/edition'; -import type { DCRBadgeType } from './badge'; -import type { Branding, EditionBranding } from './branding'; +import type { Branding, CollectionBranding, EditionBranding } from './branding'; import type { ServerSideTests, Switches } from './config'; import type { Image } from './content'; import type { FooterType } from './footer'; @@ -401,9 +400,7 @@ export type DCRCollectionType = { * will always be `false`. **/ canShowMore?: boolean; - editorialBadge?: DCRBadgeType; - paidContentBadge?: DCRBadgeType; - sponsoredContentBranding?: Branding; + collectionBranding?: CollectionBranding; targetedTerritory?: Territory; }; From 14cb3a6ab7978e6d1972acd8eb1c1f33821708af Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 5 Oct 2023 10:21:54 +0100 Subject: [PATCH 02/19] non-empty -> non-empty-array --- dotcom-rendering/src/lib/branding.ts | 17 +++++++++-------- .../src/lib/non-empty-array.test.ts | 7 +++++++ dotcom-rendering/src/lib/non-empty-array.ts | 10 ++++++++++ dotcom-rendering/src/lib/non-empty.test.ts | 7 ------- dotcom-rendering/src/lib/non-empty.ts | 10 ---------- 5 files changed, 26 insertions(+), 25 deletions(-) create mode 100644 dotcom-rendering/src/lib/non-empty-array.test.ts create mode 100644 dotcom-rendering/src/lib/non-empty-array.ts delete mode 100644 dotcom-rendering/src/lib/non-empty.test.ts delete mode 100644 dotcom-rendering/src/lib/non-empty.ts diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index 3be636dca49..7d8386920cd 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -10,8 +10,8 @@ import type { FEFrontCard } from '../types/front'; import { assertUnreachable } from './assert-unreachable'; import type { EditionId } from './edition'; import { guard } from './guard'; -import type { NonEmpty } from './non-empty'; -import { isNonEmpty } from './non-empty'; +import type { NonEmptyArray } from './non-empty-array'; +import { isNonEmptyArray } from './non-empty-array'; export const pickBrandingForEdition = ( editionBrandings: EditionBranding[], @@ -24,7 +24,7 @@ export const pickBrandingForEdition = ( const getBrandingFromCards = ( cards: FEFrontCard[], editionId: EditionId, -): NonEmpty | undefined => { +): NonEmptyArray | undefined => { const brandings: Branding[] = []; for (const card of cards) { const branding = pickBrandingForEdition( @@ -37,7 +37,7 @@ const getBrandingFromCards = ( } brandings.push(branding); } - if (!isNonEmpty(brandings)) { + if (!isNonEmptyArray(brandings)) { return undefined; } return brandings; @@ -49,9 +49,10 @@ const isCollectionBrandingKind = guard([ 'foundation', ] as const); -const getBrandingType = ([firstBranding, ...restBranding]: NonEmpty): - | BrandingKind - | undefined => { +const getBrandingType = ([ + firstBranding, + ...restBranding +]: NonEmptyArray): BrandingKind | undefined => { const name = firstBranding.brandingType?.name; if (!isCollectionBrandingKind(name)) { @@ -72,7 +73,7 @@ const getBrandingType = ([firstBranding, ...restBranding]: NonEmpty): const everyCardHasSameSponsor = ([ firstBranding, ...restBranding -]: NonEmpty): boolean => +]: NonEmptyArray): boolean => restBranding.every( (branding) => branding.sponsorName === firstBranding.sponsorName, ); diff --git a/dotcom-rendering/src/lib/non-empty-array.test.ts b/dotcom-rendering/src/lib/non-empty-array.test.ts new file mode 100644 index 00000000000..fbf12977dcd --- /dev/null +++ b/dotcom-rendering/src/lib/non-empty-array.test.ts @@ -0,0 +1,7 @@ +import { isNonEmptyArray } from './non-empty-array'; + +it('isNonEmpty', () => { + expect(isNonEmptyArray([])).toBe(false); + expect(isNonEmptyArray([1])).toBe(true); + expect(isNonEmptyArray([1, 2, 3])).toBe(true); +}); diff --git a/dotcom-rendering/src/lib/non-empty-array.ts b/dotcom-rendering/src/lib/non-empty-array.ts new file mode 100644 index 00000000000..f6417929949 --- /dev/null +++ b/dotcom-rendering/src/lib/non-empty-array.ts @@ -0,0 +1,10 @@ +/** + * Type representing an array with at least one element + */ +export type NonEmptyArray = [T, ...T[]]; + +/** + * Type guard for determining whether an array has at least one element + */ +export const isNonEmptyArray = (items: T[]): items is NonEmptyArray => + items.length > 0; diff --git a/dotcom-rendering/src/lib/non-empty.test.ts b/dotcom-rendering/src/lib/non-empty.test.ts deleted file mode 100644 index 38f32e0b7d2..00000000000 --- a/dotcom-rendering/src/lib/non-empty.test.ts +++ /dev/null @@ -1,7 +0,0 @@ -import { isNonEmpty } from './non-empty'; - -it('isNonEmpty', () => { - expect(isNonEmpty([])).toBe(false); - expect(isNonEmpty([1])).toBe(true); - expect(isNonEmpty([1, 2, 3])).toBe(true); -}); diff --git a/dotcom-rendering/src/lib/non-empty.ts b/dotcom-rendering/src/lib/non-empty.ts deleted file mode 100644 index b1f32fada2f..00000000000 --- a/dotcom-rendering/src/lib/non-empty.ts +++ /dev/null @@ -1,10 +0,0 @@ -/** - * TODO - */ -export type NonEmpty = [T, ...T[]]; - -/** - * TODO - */ -export const isNonEmpty = (items: T[]): items is NonEmpty => - items.length > 0; From 573ec2a29579d8ad27f1e6cbd84accba41b8c750 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 5 Oct 2023 10:32:21 +0100 Subject: [PATCH 03/19] Simplify the types for collection branding --- dotcom-rendering/src/types/branding.ts | 31 +++++++++++++++----------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/dotcom-rendering/src/types/branding.ts b/dotcom-rendering/src/types/branding.ts index 0f13d84d453..a22fc2161e7 100644 --- a/dotcom-rendering/src/types/branding.ts +++ b/dotcom-rendering/src/types/branding.ts @@ -33,18 +33,23 @@ export interface EditionBranding { export type BrandingKind = 'paid-content' | 'foundation' | 'sponsored'; -export type SponsorshipBranding = { - kind: BrandingKind; - isFrontBranding: boolean; - branding: Branding; -}; - -export type EditorialBranding = { - kind: 'editorial'; - badge: DCRBadgeType; -}; - /** - * The type of branding that can be applied to any given collection + * Branding that can be applied to an entire collection on a front */ -export type CollectionBranding = SponsorshipBranding | EditorialBranding; +export type CollectionBranding = + | { + kind: BrandingKind; + /** + * In certain circumstances a collection might display the branding on behalf of an entire front + * In that case this property is true + */ + isFrontBranding: boolean; + branding: Branding; + } + | { + /** + * Collections from certain series can have an 'editorial' badge selected set hardcoded in DCR + */ + kind: 'editorial'; + badge: DCRBadgeType; + }; From 280476d555cffa3299f6060605cbf9f5a22c16da Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 5 Oct 2023 11:43:31 +0100 Subject: [PATCH 04/19] Remove erroneous export --- dotcom-rendering/src/lib/branding.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index 7d8386920cd..8ead97783d9 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -189,4 +189,3 @@ export const decideCollectionBranding = ({ } } }; -export { assertUnreachable }; From 5786b94697bb68ce0cef5257f2646ff5930ebe2e Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 5 Oct 2023 12:29:51 +0100 Subject: [PATCH 05/19] Add unit tests for branding function --- dotcom-rendering/src/lib/branding.test.ts | 391 ++++++++++++++++++++++ dotcom-rendering/src/lib/branding.ts | 11 +- 2 files changed, 399 insertions(+), 3 deletions(-) create mode 100644 dotcom-rendering/src/lib/branding.test.ts diff --git a/dotcom-rendering/src/lib/branding.test.ts b/dotcom-rendering/src/lib/branding.test.ts new file mode 100644 index 00000000000..3a91a36923a --- /dev/null +++ b/dotcom-rendering/src/lib/branding.test.ts @@ -0,0 +1,391 @@ +import type { Branding } from '../types/branding'; +import { decideCollectionBranding } from './branding'; + +// For the purpose of these tests we don't care about the contents of the logo objects +const logo = {} as Branding['logo']; + +describe('decideCollectionBranding', () => { + it('use editorial badge (even with valid branding on cards) if series is defined', () => { + const cardBranding = { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }; + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + index: 3, + seriesTag: 'politics/series/road-to-the-vote', + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toStrictEqual({ + kind: 'editorial', + badge: { + imageSrc: `/static/frontend/badges/EUReferendumBadge.svg`, + href: `/politics/series/road-to-the-vote`, + }, + }); + }); + + it('correctly picks branding from a card by their edition', () => { + const cards = [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + }, + { + edition: { id: 'US' }, + branding: { + brandingType: { name: 'sponsored' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + ]; + const ukBranding = decideCollectionBranding({ + frontBranding: undefined, + index: 3, + seriesTag: undefined, + cards, + editionId: 'UK', + }); + expect(ukBranding).toMatchObject({ + kind: 'paid-content', + isFrontBranding: false, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + }); + const usBranding = decideCollectionBranding({ + frontBranding: undefined, + index: 3, + seriesTag: undefined, + cards, + editionId: 'US', + }); + expect(usBranding).toMatchObject({ + kind: 'sponsored', + isFrontBranding: false, + branding: { + brandingType: { name: 'sponsored' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + }); + }); + + it('paid content derived from multiple cards', () => { + const cardBranding = { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }; + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + index: 3, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toMatchObject({ + kind: 'paid-content', + isFrontBranding: false, + branding: cardBranding, + }); + }); + + it('cards have different branding types', () => { + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + index: 3, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'sponsored' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'foundation' }, + sponsorName: 'baz', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toBeUndefined(); + }); + + it('sponsored branding when all of the branding types are sponsored and the names match', () => { + const cardBranding = { + brandingType: { name: 'sponsored' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }; + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + index: 3, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toStrictEqual({ + kind: 'sponsored', + isFrontBranding: false, + branding: cardBranding, + }); + }); + + it('sponsored card branding with different sponsor names returns undefined', () => { + // The branding we'll apply to each card in this test + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + index: 3, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'sponsored' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'sponsored' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'sponsored' }, + sponsorName: 'baz', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toBeUndefined(); + }); + + it('returns undefined when not all cards have branding', () => { + // The branding we'll apply to each card in this test + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + index: 3, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toBeUndefined(); + }); +}); diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index 8ead97783d9..c15adc0feb1 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -6,13 +6,18 @@ import type { CollectionBranding, EditionBranding, } from '../types/branding'; -import type { FEFrontCard } from '../types/front'; import { assertUnreachable } from './assert-unreachable'; import type { EditionId } from './edition'; import { guard } from './guard'; import type { NonEmptyArray } from './non-empty-array'; import { isNonEmptyArray } from './non-empty-array'; +/** + * For the sake of determine branding on a collection, these are the only + * properties we care about for any given card + */ +type CardWithBranding = { properties: { editionBrandings: EditionBranding[] } }; + export const pickBrandingForEdition = ( editionBrandings: EditionBranding[], editionId: EditionId, @@ -22,7 +27,7 @@ export const pickBrandingForEdition = ( )?.branding; const getBrandingFromCards = ( - cards: FEFrontCard[], + cards: CardWithBranding[], editionId: EditionId, ): NonEmptyArray | undefined => { const brandings: Branding[] = []; @@ -120,7 +125,7 @@ export const decideCollectionBranding = ({ frontBranding: Branding | undefined; index: number; seriesTag: string | undefined; - cards: FEFrontCard[]; + cards: CardWithBranding[]; editionId: EditionId; }): CollectionBranding | undefined => { // TODO it might not be enough to first check the first index From 60bed50b5c9806bc1b34a330e68b5603dce7d2d0 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 5 Oct 2023 14:35:11 +0100 Subject: [PATCH 06/19] Do not assume we can place branding in first collection --- dotcom-rendering/src/lib/branding.test.ts | 167 +++++++++++++++++- dotcom-rendering/src/lib/branding.ts | 12 +- .../src/model/enhanceCollections.ts | 19 +- 3 files changed, 182 insertions(+), 16 deletions(-) diff --git a/dotcom-rendering/src/lib/branding.test.ts b/dotcom-rendering/src/lib/branding.test.ts index 3a91a36923a..2bc3c882ad2 100644 --- a/dotcom-rendering/src/lib/branding.test.ts +++ b/dotcom-rendering/src/lib/branding.test.ts @@ -14,7 +14,7 @@ describe('decideCollectionBranding', () => { }; const collectionBranding = decideCollectionBranding({ frontBranding: undefined, - index: 3, + couldDisplayFrontBranding: false, seriesTag: 'politics/series/road-to-the-vote', cards: [ { @@ -88,7 +88,7 @@ describe('decideCollectionBranding', () => { ]; const ukBranding = decideCollectionBranding({ frontBranding: undefined, - index: 3, + couldDisplayFrontBranding: false, seriesTag: undefined, cards, editionId: 'UK', @@ -105,7 +105,7 @@ describe('decideCollectionBranding', () => { }); const usBranding = decideCollectionBranding({ frontBranding: undefined, - index: 3, + couldDisplayFrontBranding: false, seriesTag: undefined, cards, editionId: 'US', @@ -131,7 +131,7 @@ describe('decideCollectionBranding', () => { }; const collectionBranding = decideCollectionBranding({ frontBranding: undefined, - index: 3, + couldDisplayFrontBranding: false, seriesTag: undefined, cards: [ { @@ -177,7 +177,7 @@ describe('decideCollectionBranding', () => { it('cards have different branding types', () => { const collectionBranding = decideCollectionBranding({ frontBranding: undefined, - index: 3, + couldDisplayFrontBranding: false, seriesTag: undefined, cards: [ { @@ -240,7 +240,7 @@ describe('decideCollectionBranding', () => { }; const collectionBranding = decideCollectionBranding({ frontBranding: undefined, - index: 3, + couldDisplayFrontBranding: false, seriesTag: undefined, cards: [ { @@ -287,7 +287,7 @@ describe('decideCollectionBranding', () => { // The branding we'll apply to each card in this test const collectionBranding = decideCollectionBranding({ frontBranding: undefined, - index: 3, + couldDisplayFrontBranding: false, seriesTag: undefined, cards: [ { @@ -345,7 +345,7 @@ describe('decideCollectionBranding', () => { // The branding we'll apply to each card in this test const collectionBranding = decideCollectionBranding({ frontBranding: undefined, - index: 3, + couldDisplayFrontBranding: false, seriesTag: undefined, cards: [ { @@ -388,4 +388,155 @@ describe('decideCollectionBranding', () => { }); expect(collectionBranding).toBeUndefined(); }); + + it('show front branding when present and possible to display', () => { + const collectionBranding = decideCollectionBranding({ + frontBranding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + couldDisplayFrontBranding: true, + seriesTag: undefined, + cards: [], + editionId: 'UK', + }); + expect(collectionBranding).toStrictEqual({ + kind: 'paid-content', + isFrontBranding: true, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + }); + }); + + it('undefined when there is front branding but not eligible for display on this collection', () => { + const collectionBranding = decideCollectionBranding({ + frontBranding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + couldDisplayFrontBranding: false, + seriesTag: undefined, + cards: [], + editionId: 'UK', + }); + expect(collectionBranding).toBeUndefined(); + }); + + it('when cards are present', () => { + const cardBranding = { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }; + const collectionBranding = decideCollectionBranding({ + frontBranding: { + brandingType: { name: 'sponsored' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + couldDisplayFrontBranding: false, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toStrictEqual({ + kind: 'paid-content', + isFrontBranding: false, + branding: cardBranding, + }); + }); + + it('do not show branding when front branding matches, but we are not displaying front branding', () => { + const cardBranding = { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }; + const collectionBranding = decideCollectionBranding({ + frontBranding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + couldDisplayFrontBranding: false, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: cardBranding, + }, + ], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toBeUndefined(); + }); }); diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index c15adc0feb1..67c5b77483d 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -88,8 +88,8 @@ const everyCardHasSameSponsor = ([ */ const brandingEqual = (b1: Branding, b2: Branding) => { return ( - b1.sponsorName === b2.sponsorName && - b1.brandingType?.name === b2.brandingType?.name + b1.brandingType?.name === b2.brandingType?.name && + b1.sponsorName === b2.sponsorName ); }; @@ -117,20 +117,18 @@ export const badgeFromBranding = ( export const decideCollectionBranding = ({ frontBranding, - index, + couldDisplayFrontBranding, seriesTag, cards, editionId, }: { frontBranding: Branding | undefined; - index: number; + couldDisplayFrontBranding: boolean; seriesTag: string | undefined; cards: CardWithBranding[]; editionId: EditionId; }): CollectionBranding | undefined => { - // TODO it might not be enough to first check the first index - // For example, what if the first container is a thrasher? - if (index === 0 && frontBranding !== undefined) { + if (couldDisplayFrontBranding && frontBranding !== undefined) { const kind = getBrandingType([frontBranding]); if (!kind) { return undefined; diff --git a/dotcom-rendering/src/model/enhanceCollections.ts b/dotcom-rendering/src/model/enhanceCollections.ts index 3793f2d0749..224b6dd8fbe 100644 --- a/dotcom-rendering/src/model/enhanceCollections.ts +++ b/dotcom-rendering/src/model/enhanceCollections.ts @@ -24,6 +24,21 @@ const isSupported = (collection: FECollectionType): boolean => ) ); +const findCollectionSuitableForFrontBranding = ( + collections: FECollectionType[], +) => { + // Find the lowest indexed front that COULD display branding + const index = collections.findIndex( + ({ collectionType }) => collectionType !== 'fixed/thrasher', + ); + // `findIndex` returns -1 when no element is found + // Treat that instead as undefined + if (index === -1) { + return undefined; + } + return index; +}; + export const enhanceCollections = ({ collections, editionId, @@ -41,13 +56,15 @@ export const enhanceCollections = ({ onPageDescription?: string; isOnPaidContentFront?: boolean; }): DCRCollectionType[] => { + const indexToShowFrontBranding = + findCollectionSuitableForFrontBranding(collections); return collections.filter(isSupported).map((collection, index) => { const { id, displayName, collectionType, hasMore, href, description } = collection; const allCards = [...collection.curated, ...collection.backfill]; const collectionBranding = decideCollectionBranding({ frontBranding, - index, + couldDisplayFrontBranding: index === indexToShowFrontBranding, seriesTag: collection.config.href, cards: allCards, editionId, From 58baf96a8dc4322840a3f6ef70e1fc8cdf511bc9 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 5 Oct 2023 16:16:36 +0100 Subject: [PATCH 07/19] Add some stories for front sections with branding --- .../src/components/FrontSection.stories.tsx | 141 +++++++++++++----- 1 file changed, 100 insertions(+), 41 deletions(-) diff --git a/dotcom-rendering/src/components/FrontSection.stories.tsx b/dotcom-rendering/src/components/FrontSection.stories.tsx index 06f42213c66..a1ac86f4c55 100644 --- a/dotcom-rendering/src/components/FrontSection.stories.tsx +++ b/dotcom-rendering/src/components/FrontSection.stories.tsx @@ -242,53 +242,112 @@ export const TreatsStory = () => { }; TreatsStory.storyName = 'with treats and date header'; +export const WithEditorialBadge = () => { + return ( + + + + ); +}; +WithEditorialBadge.storyName = 'with editorial badge'; + /** - * Note only the first container should show a badge + * Use the same logo for each of the stories with branding */ -export const MultipleOnAPaidFront = () => { - // const frontBranding = { - // edition: { - // id: 'UK', - // }, - // branding: { - // brandingType: { - // name: 'paid-content', - // }, - // sponsorName: 'Grounded', - // logo: { - // src: 'https://static.theguardian.com/commercial/sponsor/28/Oct/2020/daa941da-14fd-46cc-85cb-731ce59050ee-Grounded_badging-280x180.png', - // dimensions: { - // width: 140, - // height: 90, - // }, - // link: '/', - // label: 'Paid for by', - // }, - // aboutThisLink: - // 'https://www.theguardian.com/info/2016/jan/25/content-funding', - // }, - // } as const; +const logo = { + src: 'https://static.theguardian.com/commercial/sponsor/28/Oct/2020/daa941da-14fd-46cc-85cb-731ce59050ee-Grounded_badging-280x180.png', + dimensions: { + width: 140, + height: 90, + }, + link: '/', + label: 'Paid for by', +}; +export const WithSponsoredBranding = () => { return ( - <> - - - - - - - + + + + ); +}; +WithSponsoredBranding.storyName = 'with sponsored branding'; + +export const WithPaidBranding = () => { + return ( + + + + ); +}; +WithPaidBranding.storyName = 'with paid content branding'; + +export const WithPaidContentForWholeFront = () => { + return ( + + + ); }; -MultipleOnAPaidFront.storyName = 'two sections on a paid front'; +WithPaidContentForWholeFront.storyName = 'with paid content for whole front'; export const PageSkinStory = () => { return ( From 3ce3c7208e0cb5e3743aa47106d7ef0886f25d06 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 5 Oct 2023 16:38:50 +0100 Subject: [PATCH 08/19] Use as const instead of The version of stylelint we're using flags as a syntax error. --- dotcom-rendering/src/lib/branding.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dotcom-rendering/src/lib/branding.test.ts b/dotcom-rendering/src/lib/branding.test.ts index 2bc3c882ad2..9f8ffd2135d 100644 --- a/dotcom-rendering/src/lib/branding.test.ts +++ b/dotcom-rendering/src/lib/branding.test.ts @@ -65,7 +65,7 @@ describe('decideCollectionBranding', () => { properties: { editionBrandings: [ { - edition: { id: 'UK' }, + edition: { id: 'UK' as const }, branding: { brandingType: { name: 'paid-content' }, sponsorName: 'foo', @@ -74,7 +74,7 @@ describe('decideCollectionBranding', () => { }, }, { - edition: { id: 'US' }, + edition: { id: 'US' as const }, branding: { brandingType: { name: 'sponsored' }, sponsorName: 'bar', From 04488be64853b2d4b20336a06473a8715e64b815 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Wed, 11 Oct 2023 09:51:38 +0100 Subject: [PATCH 09/19] Check sponsor name for all non-editorial kinds Add a couple of unit tests to assert the new behaviour, and adjust the descriptions of some of the test. --- dotcom-rendering/src/lib/branding.test.ts | 117 +++++++++++++++++++--- dotcom-rendering/src/lib/branding.ts | 33 ++---- 2 files changed, 113 insertions(+), 37 deletions(-) diff --git a/dotcom-rendering/src/lib/branding.test.ts b/dotcom-rendering/src/lib/branding.test.ts index 9f8ffd2135d..7d850ed9a58 100644 --- a/dotcom-rendering/src/lib/branding.test.ts +++ b/dotcom-rendering/src/lib/branding.test.ts @@ -59,7 +59,7 @@ describe('decideCollectionBranding', () => { }); }); - it('correctly picks branding from a card by their edition', () => { + it('picks branding from a card by their edition', () => { const cards = [ { properties: { @@ -122,7 +122,7 @@ describe('decideCollectionBranding', () => { }); }); - it('paid content derived from multiple cards', () => { + it('is paid content derived from multiple cards', () => { const cardBranding = { brandingType: { name: 'paid-content' }, sponsorName: 'foo', @@ -174,7 +174,55 @@ describe('decideCollectionBranding', () => { }); }); - it('cards have different branding types', () => { + it('undefined when not all cards have branding', () => { + // The branding we'll apply to each card in this test + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + couldDisplayFrontBranding: false, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toBeUndefined(); + }); + + it('is undefined when cards have different branding types', () => { const collectionBranding = decideCollectionBranding({ frontBranding: undefined, couldDisplayFrontBranding: false, @@ -231,7 +279,7 @@ describe('decideCollectionBranding', () => { expect(collectionBranding).toBeUndefined(); }); - it('sponsored branding when all of the branding types are sponsored and the names match', () => { + it('is sponsored branding when all of the branding types are sponsored and the names match', () => { const cardBranding = { brandingType: { name: 'sponsored' }, sponsorName: 'foo', @@ -283,7 +331,7 @@ describe('decideCollectionBranding', () => { }); }); - it('sponsored card branding with different sponsor names returns undefined', () => { + it('is undefined when branding cards are sponsored and have different sponsor names', () => { // The branding we'll apply to each card in this test const collectionBranding = decideCollectionBranding({ frontBranding: undefined, @@ -341,8 +389,7 @@ describe('decideCollectionBranding', () => { expect(collectionBranding).toBeUndefined(); }); - it('returns undefined when not all cards have branding', () => { - // The branding we'll apply to each card in this test + it('is paid content branding when all of the branding types are paid-content and the names match', () => { const collectionBranding = decideCollectionBranding({ frontBranding: undefined, couldDisplayFrontBranding: false, @@ -370,7 +417,7 @@ describe('decideCollectionBranding', () => { edition: { id: 'UK' }, branding: { brandingType: { name: 'paid-content' }, - sponsorName: 'bar', + sponsorName: 'foo', aboutThisLink: '', logo, }, @@ -378,9 +425,55 @@ describe('decideCollectionBranding', () => { ], }, }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toStrictEqual({ + kind: 'paid-content', + isFrontBranding: false, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + }); + }); + + it('is undefined when branding cards are paid-content and have different sponsor names', () => { + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + couldDisplayFrontBranding: false, + seriesTag: undefined, + cards: [ { properties: { - editionBrandings: [], + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'foo', + aboutThisLink: '', + logo, + }, + }, + ], + }, + }, + { + properties: { + editionBrandings: [ + { + edition: { id: 'UK' }, + branding: { + brandingType: { name: 'paid-content' }, + sponsorName: 'bar', + aboutThisLink: '', + logo, + }, + }, + ], }, }, ], @@ -389,7 +482,7 @@ describe('decideCollectionBranding', () => { expect(collectionBranding).toBeUndefined(); }); - it('show front branding when present and possible to display', () => { + it('is front branding when present and possible to display', () => { const collectionBranding = decideCollectionBranding({ frontBranding: { brandingType: { name: 'paid-content' }, @@ -414,7 +507,7 @@ describe('decideCollectionBranding', () => { }); }); - it('undefined when there is front branding but not eligible for display on this collection', () => { + it('is undefined when there is front branding (and no card branding) that is not eligible for display on this collection', () => { const collectionBranding = decideCollectionBranding({ frontBranding: { brandingType: { name: 'paid-content' }, @@ -487,7 +580,7 @@ describe('decideCollectionBranding', () => { }); }); - it('do not show branding when front branding matches, but we are not displaying front branding', () => { + it('is undefined when front branding matches card branding, but we are not displaying front branding', () => { const cardBranding = { brandingType: { name: 'paid-content' }, sponsorName: 'foo', diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index 67c5b77483d..aaaef8869db 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -6,7 +6,6 @@ import type { CollectionBranding, EditionBranding, } from '../types/branding'; -import { assertUnreachable } from './assert-unreachable'; import type { EditionId } from './edition'; import { guard } from './guard'; import type { NonEmptyArray } from './non-empty-array'; @@ -166,29 +165,13 @@ export const decideCollectionBranding = ({ return undefined; } - switch (kind) { - case 'foundation': - case 'paid-content': { - return { - kind, - isFrontBranding: false, - branding, - }; - } - case 'sponsored': { - // We do an additional check on sponsored branding - // Ensuring that each has the same sponsor name - if (!everyCardHasSameSponsor(brandingForCards)) { - return undefined; - } - return { - kind, - isFrontBranding: false, - branding, - }; - } - default: { - return assertUnreachable(kind); - } + if (!everyCardHasSameSponsor(brandingForCards)) { + return undefined; } + + return { + kind, + isFrontBranding: false, + branding, + }; }; From 599a51e96608178af53b19b9cba3c0a1b6bc4939 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Wed, 11 Oct 2023 10:27:39 +0100 Subject: [PATCH 10/19] Add some comments to lib/branding.ts --- dotcom-rendering/src/lib/branding.ts | 32 ++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index aaaef8869db..66c2ce3fcec 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -6,13 +6,14 @@ import type { CollectionBranding, EditionBranding, } from '../types/branding'; +import { assertUnreachable } from './assert-unreachable'; import type { EditionId } from './edition'; import { guard } from './guard'; import type { NonEmptyArray } from './non-empty-array'; import { isNonEmptyArray } from './non-empty-array'; /** - * For the sake of determine branding on a collection, these are the only + * For the sake of determining branding on a collection, these are the only * properties we care about for any given card */ type CardWithBranding = { properties: { editionBrandings: EditionBranding[] } }; @@ -25,6 +26,12 @@ export const pickBrandingForEdition = ( ({ edition, branding }) => edition.id === editionId && branding, )?.branding; +/** + * Retrieve the branding object from each of the cards in an array of cards, for a given edition + * + * @returns `undefined` if AT LEAST ONE of the cards is missing branding, + * otherwise returns a non-empty array of branding + */ const getBrandingFromCards = ( cards: CardWithBranding[], editionId: EditionId, @@ -74,6 +81,9 @@ const getBrandingType = ([ return name; }; +/** + * Check each branding has the same sponsor name + */ const everyCardHasSameSponsor = ([ firstBranding, ...restBranding @@ -111,6 +121,9 @@ export const badgeFromBranding = ( case undefined: { return undefined; } + default: { + return assertUnreachable(collectionBranding); + } } }; @@ -127,6 +140,8 @@ export const decideCollectionBranding = ({ cards: CardWithBranding[]; editionId: EditionId; }): CollectionBranding | undefined => { + // If this collection is eligible to display front branding + // AND there is front branding defined, we should display it if (couldDisplayFrontBranding && frontBranding !== undefined) { const kind = getBrandingType([frontBranding]); if (!kind) { @@ -139,6 +154,7 @@ export const decideCollectionBranding = ({ }; } + // If the series tag of this collection matches an editorial badge, we should use that const editorialBadge = decideEditorialBadge(seriesTag); if (editorialBadge) { return { @@ -147,24 +163,32 @@ export const decideCollectionBranding = ({ }; } + // Retrieve an array of branding from the cards that belong to the collection + // If this is valid (aka not undefined), then we can use it to derive the + // branding of the collection const brandingForCards = getBrandingFromCards(cards, editionId); - if (!brandingForCards) { return undefined; } const kind = getBrandingType(brandingForCards); - if (!kind) { return undefined; } const [branding] = brandingForCards; - if (frontBranding && brandingEqual(frontBranding, branding)) { + // If this collection belongs to a front that has branding, and the branding + // derived from the cards is the same, then don't display this branding. + // This takes care of the case when another card is displaying the branding + // on behalf of the whole front and this collection is further down the + // front, with its branding hidden + if (frontBranding !== undefined && brandingEqual(frontBranding, branding)) { return undefined; } + // Ensure each of the card's branding has the same sponsor + if (!everyCardHasSameSponsor(brandingForCards)) { return undefined; } From 90f8f3f2eee271020280d6b49cfe389f6a7fdd4b Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Thu, 12 Oct 2023 10:13:08 +0100 Subject: [PATCH 11/19] Derive branding kind from type --- dotcom-rendering/src/lib/branding.test.ts | 14 +++++++------- dotcom-rendering/src/lib/branding.ts | 9 +-------- dotcom-rendering/src/types/branding.ts | 2 +- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/dotcom-rendering/src/lib/branding.test.ts b/dotcom-rendering/src/lib/branding.test.ts index 7d850ed9a58..b20524ab1e7 100644 --- a/dotcom-rendering/src/lib/branding.test.ts +++ b/dotcom-rendering/src/lib/branding.test.ts @@ -7,7 +7,7 @@ const logo = {} as Branding['logo']; describe('decideCollectionBranding', () => { it('use editorial badge (even with valid branding on cards) if series is defined', () => { const cardBranding = { - brandingType: { name: 'paid-content' }, + brandingType: { name: 'paid-content' as const }, sponsorName: 'foo', aboutThisLink: '', logo, @@ -67,7 +67,7 @@ describe('decideCollectionBranding', () => { { edition: { id: 'UK' as const }, branding: { - brandingType: { name: 'paid-content' }, + brandingType: { name: 'paid-content' as const }, sponsorName: 'foo', aboutThisLink: '', logo, @@ -76,7 +76,7 @@ describe('decideCollectionBranding', () => { { edition: { id: 'US' as const }, branding: { - brandingType: { name: 'sponsored' }, + brandingType: { name: 'sponsored' as const }, sponsorName: 'bar', aboutThisLink: '', logo, @@ -124,7 +124,7 @@ describe('decideCollectionBranding', () => { it('is paid content derived from multiple cards', () => { const cardBranding = { - brandingType: { name: 'paid-content' }, + brandingType: { name: 'paid-content' as const }, sponsorName: 'foo', aboutThisLink: '', logo, @@ -281,7 +281,7 @@ describe('decideCollectionBranding', () => { it('is sponsored branding when all of the branding types are sponsored and the names match', () => { const cardBranding = { - brandingType: { name: 'sponsored' }, + brandingType: { name: 'sponsored' as const }, sponsorName: 'foo', aboutThisLink: '', logo, @@ -525,7 +525,7 @@ describe('decideCollectionBranding', () => { it('when cards are present', () => { const cardBranding = { - brandingType: { name: 'paid-content' }, + brandingType: { name: 'paid-content' as const }, sponsorName: 'foo', aboutThisLink: '', logo, @@ -582,7 +582,7 @@ describe('decideCollectionBranding', () => { it('is undefined when front branding matches card branding, but we are not displaying front branding', () => { const cardBranding = { - brandingType: { name: 'paid-content' }, + brandingType: { name: 'paid-content' as const }, sponsorName: 'foo', aboutThisLink: '', logo, diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index 66c2ce3fcec..6970796146a 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -8,7 +8,6 @@ import type { } from '../types/branding'; import { assertUnreachable } from './assert-unreachable'; import type { EditionId } from './edition'; -import { guard } from './guard'; import type { NonEmptyArray } from './non-empty-array'; import { isNonEmptyArray } from './non-empty-array'; @@ -54,19 +53,13 @@ const getBrandingFromCards = ( return brandings; }; -const isCollectionBrandingKind = guard([ - 'paid-content', - 'sponsored', - 'foundation', -] as const); - const getBrandingType = ([ firstBranding, ...restBranding ]: NonEmptyArray): BrandingKind | undefined => { const name = firstBranding.brandingType?.name; - if (!isCollectionBrandingKind(name)) { + if (!name) { return undefined; } diff --git a/dotcom-rendering/src/types/branding.ts b/dotcom-rendering/src/types/branding.ts index a22fc2161e7..a3c99adff17 100644 --- a/dotcom-rendering/src/types/branding.ts +++ b/dotcom-rendering/src/types/branding.ts @@ -31,7 +31,7 @@ export interface EditionBranding { branding?: Branding; } -export type BrandingKind = 'paid-content' | 'foundation' | 'sponsored'; +export type BrandingKind = BrandingType['name']; /** * Branding that can be applied to an entire collection on a front From 7dcc6f74390a703f4aab3e362ef5579b2b236e6c Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 17 Oct 2023 09:49:07 +0100 Subject: [PATCH 12/19] Fix typo in comment in types/branding.ts --- dotcom-rendering/src/types/branding.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dotcom-rendering/src/types/branding.ts b/dotcom-rendering/src/types/branding.ts index a3c99adff17..a199b87c3ac 100644 --- a/dotcom-rendering/src/types/branding.ts +++ b/dotcom-rendering/src/types/branding.ts @@ -48,7 +48,7 @@ export type CollectionBranding = } | { /** - * Collections from certain series can have an 'editorial' badge selected set hardcoded in DCR + * Collections from certain series can have an 'editorial' badge selected from a hardcoded set */ kind: 'editorial'; badge: DCRBadgeType; From a0624ff36ace6056e5b51a1c08ab52d10c94fe65 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 17 Oct 2023 09:53:18 +0100 Subject: [PATCH 13/19] Correct fn name in decideEditorialBadge test --- .../src/model/decideEditorialBadge.test.ts | 62 +++++++++---------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/dotcom-rendering/src/model/decideEditorialBadge.test.ts b/dotcom-rendering/src/model/decideEditorialBadge.test.ts index 48f7c5fc5d9..02b67d82d7d 100644 --- a/dotcom-rendering/src/model/decideEditorialBadge.test.ts +++ b/dotcom-rendering/src/model/decideEditorialBadge.test.ts @@ -2,40 +2,38 @@ import { decideEditorialBadge } from './decideEditorialBadge'; jest.mock('./badges'); -describe('Decide badge', () => { - describe('getBadgeFromSeriesTag', () => { - it('returns correct standard badge', () => { - const tagId = 'uk-news/series/the-brexit-gamble'; - const expectedResult = { - href: `/${tagId}`, - imageSrc: `/static/frontend/badges/EUReferendumBadge.svg`, - }; - const result = decideEditorialBadge(tagId); - expect(result).toMatchObject(expectedResult); - }); +describe('decideEditorialBadge', () => { + it('returns correct standard badge', () => { + const tagId = 'uk-news/series/the-brexit-gamble'; + const expectedResult = { + href: `/${tagId}`, + imageSrc: `/static/frontend/badges/EUReferendumBadge.svg`, + }; + const result = decideEditorialBadge(tagId); + expect(result).toMatchObject(expectedResult); + }); - it('returns correct special badge', () => { - const tagId = 'tone/newsletter-tone'; - const expectedResult = { - href: `/${tagId}`, - imageSrc: `/static/frontend/badges/newsletter-badge.svg`, - }; - const result = decideEditorialBadge(tagId); - expect(result).toMatchObject(expectedResult); - }); + it('returns correct special badge', () => { + const tagId = 'tone/newsletter-tone'; + const expectedResult = { + href: `/${tagId}`, + imageSrc: `/static/frontend/badges/newsletter-badge.svg`, + }; + const result = decideEditorialBadge(tagId); + expect(result).toMatchObject(expectedResult); + }); - it('returns undefined if no standard or special badge match found for series tag', () => { - const tagId = 'lifeandstyle/home-and-garden'; - const expectedResult = undefined; - const result = decideEditorialBadge(tagId); - expect(result).toEqual(expectedResult); - }); + it('returns undefined if no standard or special badge match found for series tag', () => { + const tagId = 'lifeandstyle/home-and-garden'; + const expectedResult = undefined; + const result = decideEditorialBadge(tagId); + expect(result).toEqual(expectedResult); + }); - it('returns undefined for undefined series tag', () => { - const tagId = undefined; - const expectedResult = undefined; - const result = decideEditorialBadge(tagId); - expect(result).toEqual(expectedResult); - }); + it('returns undefined for undefined series tag', () => { + const tagId = undefined; + const expectedResult = undefined; + const result = decideEditorialBadge(tagId); + expect(result).toEqual(expectedResult); }); }); From a8bdb66babd72e753506efb8647a08f87ec0759d Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 17 Oct 2023 09:57:02 +0100 Subject: [PATCH 14/19] Remove erroneous whitespace --- dotcom-rendering/src/lib/branding.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index 6970796146a..ee3586e300f 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -181,7 +181,6 @@ export const decideCollectionBranding = ({ } // Ensure each of the card's branding has the same sponsor - if (!everyCardHasSameSponsor(brandingForCards)) { return undefined; } From f147a5a9d22b6789d27da148f7116997180186b5 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 24 Oct 2023 15:32:21 +0100 Subject: [PATCH 15/19] Use NonEmptyArray type from tuple.ts --- dotcom-rendering/src/lib/branding.ts | 4 ++-- dotcom-rendering/src/lib/non-empty-array.test.ts | 7 ------- dotcom-rendering/src/lib/non-empty-array.ts | 10 ---------- dotcom-rendering/src/lib/tuple.test.ts | 8 +++++++- dotcom-rendering/src/lib/tuple.ts | 6 ++++++ 5 files changed, 15 insertions(+), 20 deletions(-) delete mode 100644 dotcom-rendering/src/lib/non-empty-array.test.ts delete mode 100644 dotcom-rendering/src/lib/non-empty-array.ts diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index ee3586e300f..a34ecc9de85 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -8,8 +8,8 @@ import type { } from '../types/branding'; import { assertUnreachable } from './assert-unreachable'; import type { EditionId } from './edition'; -import type { NonEmptyArray } from './non-empty-array'; -import { isNonEmptyArray } from './non-empty-array'; +import type { NonEmptyArray } from './tuple'; +import { isNonEmptyArray } from './tuple'; /** * For the sake of determining branding on a collection, these are the only diff --git a/dotcom-rendering/src/lib/non-empty-array.test.ts b/dotcom-rendering/src/lib/non-empty-array.test.ts deleted file mode 100644 index fbf12977dcd..00000000000 --- a/dotcom-rendering/src/lib/non-empty-array.test.ts +++ /dev/null @@ -1,7 +0,0 @@ -import { isNonEmptyArray } from './non-empty-array'; - -it('isNonEmpty', () => { - expect(isNonEmptyArray([])).toBe(false); - expect(isNonEmptyArray([1])).toBe(true); - expect(isNonEmptyArray([1, 2, 3])).toBe(true); -}); diff --git a/dotcom-rendering/src/lib/non-empty-array.ts b/dotcom-rendering/src/lib/non-empty-array.ts deleted file mode 100644 index f6417929949..00000000000 --- a/dotcom-rendering/src/lib/non-empty-array.ts +++ /dev/null @@ -1,10 +0,0 @@ -/** - * Type representing an array with at least one element - */ -export type NonEmptyArray = [T, ...T[]]; - -/** - * Type guard for determining whether an array has at least one element - */ -export const isNonEmptyArray = (items: T[]): items is NonEmptyArray => - items.length > 0; diff --git a/dotcom-rendering/src/lib/tuple.test.ts b/dotcom-rendering/src/lib/tuple.test.ts index 8751a04161c..33b398afb3d 100644 --- a/dotcom-rendering/src/lib/tuple.test.ts +++ b/dotcom-rendering/src/lib/tuple.test.ts @@ -1,4 +1,4 @@ -import { takeFirst } from './tuple'; +import { isNonEmptyArray, takeFirst } from './tuple'; describe('takeFirst', () => { it('Always returns the correct array length when the array is one less, the same as, or one more than n', () => { @@ -81,3 +81,9 @@ describe('takeFirst', () => { expect(results[35].length).toEqual(11); }); }); + +it('isNonEmptyArray', () => { + expect(isNonEmptyArray([])).toBe(false); + expect(isNonEmptyArray([1])).toBe(true); + expect(isNonEmptyArray([1, 2, 3])).toBe(true); +}); diff --git a/dotcom-rendering/src/lib/tuple.ts b/dotcom-rendering/src/lib/tuple.ts index 0aa18c0de6d..f46260e8757 100644 --- a/dotcom-rendering/src/lib/tuple.ts +++ b/dotcom-rendering/src/lib/tuple.ts @@ -87,3 +87,9 @@ export const takeFirst = < * Type representing an array with at least one element */ export type NonEmptyArray = [T, ...T[]]; + +/** + * Type guard for determining whether an array has at least one element + */ +export const isNonEmptyArray = (items: T[]): items is NonEmptyArray => + items.length > 0; From 86660ba191f91a913e5229cb3c6f23b63ce7195a Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 24 Oct 2023 15:33:12 +0100 Subject: [PATCH 16/19] Fix typo in comment in findCollectionSuitableForFrontBranding Co-authored-by: Charlotte Emms <43961396+cemms1@users.noreply.github.com> --- dotcom-rendering/src/model/enhanceCollections.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dotcom-rendering/src/model/enhanceCollections.ts b/dotcom-rendering/src/model/enhanceCollections.ts index 224b6dd8fbe..72006d32c69 100644 --- a/dotcom-rendering/src/model/enhanceCollections.ts +++ b/dotcom-rendering/src/model/enhanceCollections.ts @@ -27,7 +27,7 @@ const isSupported = (collection: FECollectionType): boolean => const findCollectionSuitableForFrontBranding = ( collections: FECollectionType[], ) => { - // Find the lowest indexed front that COULD display branding + // Find the lowest indexed collection that COULD display branding const index = collections.findIndex( ({ collectionType }) => collectionType !== 'fixed/thrasher', ); From 919bf515fccf7e105428d7f6b521e54475dc3066 Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 24 Oct 2023 15:52:36 +0100 Subject: [PATCH 17/19] Add a test for when all cards have no branding --- dotcom-rendering/src/lib/branding.test.ts | 27 +++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/dotcom-rendering/src/lib/branding.test.ts b/dotcom-rendering/src/lib/branding.test.ts index b20524ab1e7..fbc896690fe 100644 --- a/dotcom-rendering/src/lib/branding.test.ts +++ b/dotcom-rendering/src/lib/branding.test.ts @@ -222,6 +222,33 @@ describe('decideCollectionBranding', () => { expect(collectionBranding).toBeUndefined(); }); + it('is undefined when no cards have branding', () => { + const collectionBranding = decideCollectionBranding({ + frontBranding: undefined, + couldDisplayFrontBranding: false, + seriesTag: undefined, + cards: [ + { + properties: { + editionBrandings: [], + }, + }, + { + properties: { + editionBrandings: [], + }, + }, + { + properties: { + editionBrandings: [], + }, + }, + ], + editionId: 'UK', + }); + expect(collectionBranding).toBeUndefined(); + }); + it('is undefined when cards have different branding types', () => { const collectionBranding = decideCollectionBranding({ frontBranding: undefined, From ac0618f63dd4883f2e550acffe79f8f144bf198e Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Tue, 24 Oct 2023 16:46:09 +0100 Subject: [PATCH 18/19] Inline BrandingType['name'] --- dotcom-rendering/src/lib/branding.ts | 4 ++-- dotcom-rendering/src/types/branding.ts | 12 +++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/dotcom-rendering/src/lib/branding.ts b/dotcom-rendering/src/lib/branding.ts index a34ecc9de85..a84c4f7cf0b 100644 --- a/dotcom-rendering/src/lib/branding.ts +++ b/dotcom-rendering/src/lib/branding.ts @@ -2,7 +2,7 @@ import { decideEditorialBadge } from '../model/decideEditorialBadge'; import type { DCRBadgeType } from '../types/badge'; import type { Branding, - BrandingKind, + BrandingType, CollectionBranding, EditionBranding, } from '../types/branding'; @@ -56,7 +56,7 @@ const getBrandingFromCards = ( const getBrandingType = ([ firstBranding, ...restBranding -]: NonEmptyArray): BrandingKind | undefined => { +]: NonEmptyArray): BrandingType['name'] | undefined => { const name = firstBranding.brandingType?.name; if (!name) { diff --git a/dotcom-rendering/src/types/branding.ts b/dotcom-rendering/src/types/branding.ts index a199b87c3ac..01d70221440 100644 --- a/dotcom-rendering/src/types/branding.ts +++ b/dotcom-rendering/src/types/branding.ts @@ -31,14 +31,20 @@ export interface EditionBranding { branding?: Branding; } -export type BrandingKind = BrandingType['name']; - /** * Branding that can be applied to an entire collection on a front + * + * The `kind` property here is used to disambiguate the kind of branding + * a collection can have: + * - Those funded by a third party + * - Those that have an editorial badge from a hardcoded set */ export type CollectionBranding = | { - kind: BrandingKind; + /** + * A collection can have branding that is funded by a third party + */ + kind: BrandingType['name']; /** * In certain circumstances a collection might display the branding on behalf of an entire front * In that case this property is true From f03c56490a2b7325ff4caad9cd4d3fbc2dfc215f Mon Sep 17 00:00:00 2001 From: Chris Jones Date: Wed, 25 Oct 2023 10:14:59 +0100 Subject: [PATCH 19/19] fix: No links on paid fronts only --- dotcom-rendering/src/components/FrontSection.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dotcom-rendering/src/components/FrontSection.tsx b/dotcom-rendering/src/components/FrontSection.tsx index d38fcca2b4c..f891aa24281 100644 --- a/dotcom-rendering/src/components/FrontSection.tsx +++ b/dotcom-rendering/src/components/FrontSection.tsx @@ -527,7 +527,7 @@ export const FrontSection = ({ fontColour={overrides?.text.container} description={description} // On paid fronts the title is not treated as a link - url={isOnPaidContentFront ? url : undefined} + url={!isOnPaidContentFront ? url : undefined} containerPalette={containerPalette} showDateHeader={showDateHeader} editionId={editionId}