use components rather than functions for fronts ads (#13134)

guardian · Jan 14, 2025 · fa64a6e · fa64a6e
1 parent 79aceda
commit fa64a6e
Show file tree

Hide file tree

Showing 5 changed files with 271 additions and 252 deletions.
diff --git a/dotcom-rendering/src/components/DecideFrontsAdSlots.test.ts b/dotcom-rendering/src/components/DecideFrontsAdSlots.test.ts
diff --git a/dotcom-rendering/src/components/FrontsAdSlots.test.tsx b/dotcom-rendering/src/components/FrontsAdSlots.test.tsx
@@ -0,0 +1,88 @@
+import { render } from '@testing-library/react';
+import { FrontsBannerAdSlot, MerchHighOrMobileAdSlot } from './FrontsAdSlots';
+
+describe('MerchHighOrMobileAdSlot', () => {
+	it("should return null if we shouldn't render ads", () => {
+		const { container } = render(
+			<MerchHighOrMobileAdSlot
+				renderAds={false}
+				index={4}
+				collectionCount={10}
+				isPaidContent={false}
+				mobileAdPositions={[]}
+				hasPageSkin={false}
+			/>,
+		);
+
+		expect(container.innerHTML).toBe('');
+	});
+});
+
+describe('FrontsBannerAdSlot', () => {
+	it("should return null if we shouldn't render ads", () => {
+		const { container } = render(
+			<FrontsBannerAdSlot
+				renderAds={false}
+				hasPageSkin={false}
+				index={2}
+				desktopAdPositions={[2, 5]}
+			/>,
+		);
+
+		expect(container.innerHTML).toBe('');
+	});
+
+	it("should return null if there's a pageskin", () => {
+		const { container } = render(
+			<FrontsBannerAdSlot
+				renderAds={true}
+				hasPageSkin={true}
+				index={2}
+				desktopAdPositions={[2, 5]}
+			/>,
+		);
+
+		expect(container.innerHTML).toBe('');
+	});
+
+	test.each([
+		[[2, 5], 3],
+		[[2, 5], 0],
+		[[], 1],
+	])(
+		'should return null if desktopAdPositions %p does NOT contain index %i',
+		(adPositions, i) => {
+			const { container } = render(
+				<FrontsBannerAdSlot
+					renderAds={true}
+					hasPageSkin={false}
+					index={i}
+					desktopAdPositions={adPositions}
+				/>,
+			);
+
+			expect(container.innerHTML).toBe('');
+		},
+	);
+
+	test.each([
+		[[2, 5], 2],
+		[[2, 5], 5],
+		[[1], 1],
+	])(
+		'should NOT return null if desktopAdPositions %p contains index %i',
+		(adPositions, i) => {
+			const { container } = render(
+				<FrontsBannerAdSlot
+					renderAds={true}
+					hasPageSkin={false}
+					index={i}
+					desktopAdPositions={adPositions}
+				/>,
+			);
+
+			expect(container.innerHTML).not.toBe('');
+			expect(container.innerHTML).toMatch('ad-slot-container');
+		},
+	);
+});
diff --git a/...ng/src/components/DecideFrontsAdSlots.tsx → ...endering/src/components/FrontsAdSlots.tsx b/...ng/src/components/DecideFrontsAdSlots.tsx → ...endering/src/components/FrontsAdSlots.tsx
@@ -1,18 +1,24 @@
 import { Hide } from '@guardian/source/react-components';
-import type { ReactNode } from 'react';
 import { getMerchHighPosition } from '../lib/getFrontsAdPositions';
 import { palette as themePalette } from '../palette';
 import { AdSlot } from './AdSlot.web';
 import { Section } from './Section';
 
-export const decideMerchHighAndMobileAdSlots = (
-	renderAds: boolean,
-	index: number,
-	collectionCount: number,
-	isPaidContent: boolean | undefined,
-	mobileAdPositions: (number | undefined)[],
-	hasPageSkin: boolean,
-) => {
+export const MerchHighOrMobileAdSlot = ({
+	renderAds,
+	index,
+	collectionCount,
+	isPaidContent,
+	mobileAdPositions,
+	hasPageSkin,
+}: {
+	renderAds: boolean;
+	index: number;
+	collectionCount: number;
+	isPaidContent: boolean | undefined;
+	mobileAdPositions: (number | undefined)[];
+	hasPageSkin: boolean;
+}) => {
 	if (!renderAds) return null;
 
 	const minContainers = isPaidContent ? 1 : 2;
@@ -52,12 +58,15 @@ export const decideMerchHighAndMobileAdSlots = (
  * Page skins are only active above desktop breakpoints
  *
  */
-export const decideMerchandisingSlot = (
-	renderAds: boolean,
-	hasPageSkin: boolean,
-) => {
+export const MerchandisingSlot = ({
+	renderAds,
+	hasPageSkin,
+}: {
+	renderAds: boolean;
+	hasPageSkin: boolean;
+}) => {
 	if (!renderAds) return null;
-	const MerchandisingSection = ({ children }: { children: ReactNode }) => (
+	return (
 		<Section
 			fullWidth={true}
 			data-print-layout="hide"
@@ -67,38 +76,38 @@ export const decideMerchandisingSlot = (
 			backgroundColour={themePalette('--article-section-background')}
 			element="aside"
 		>
-			{children}
-		</Section>
-	);
-	return hasPageSkin ? (
-		<MerchandisingSection>
-			<Hide from="desktop">
+			{hasPageSkin ? (
+				<Hide from="desktop">
+					<AdSlot data-print-layout="hide" position="merchandising" />
+				</Hide>
+			) : (
 				<AdSlot data-print-layout="hide" position="merchandising" />
-			</Hide>
-		</MerchandisingSection>
-	) : (
-		<MerchandisingSection>
-			<AdSlot data-print-layout="hide" position="merchandising" />
-		</MerchandisingSection>
+			)}
+		</Section>
 	);
 };
 
 /**
  * Renders a fronts-banner ad when in the fronts banner AB test.
  * Only applies to network fronts on desktop screens and wider.
  */
-export const decideFrontsBannerAdSlot = (
-	renderAds: boolean,
-	hasPageSkin: boolean,
-	index: number,
-	desktopAdPositions: number[],
-) => {
+export const FrontsBannerAdSlot = ({
+	renderAds,
+	hasPageSkin,
+	index,
+	desktopAdPositions,
+}: {
+	renderAds: boolean;
+	hasPageSkin: boolean;
+	index: number;
+	desktopAdPositions: number[];
+}) => {
 	if (!renderAds || hasPageSkin) {
 		return null;
 	}
 
 	if (desktopAdPositions.includes(index)) {
-		const adIndex = desktopAdPositions.findIndex((_) => _ === index);
+		const adIndex = desktopAdPositions.indexOf(index);
 		if (adIndex === -1) return null;
 
 		return (