From 152e323d1854ee8dc2934eb7c61b1ea310ad04ac Mon Sep 17 00:00:00 2001 From: Max Duval Date: Tue, 16 Jan 2024 10:04:40 +0000 Subject: [PATCH] refactor(YoutubeAtom): no reliance on elementId we can generate a unique elementI from an increasing integer on the client. On the server this defaults to 0 or `undefined` --- .../tests/parallel-5/atom.video.e2e.spec.ts | 13 +++-- dotcom-rendering/src/components/Card/Card.tsx | 4 -- .../YoutubeAtom/YoutubeAtom.stories.tsx | 56 +++++++++---------- .../YoutubeAtom/YoutubeAtom.test.tsx | 18 +++--- .../components/YoutubeAtom/YoutubeAtom.tsx | 29 ++++++---- .../YoutubeBlockComponent.importable.tsx | 22 +++++--- .../YoutubeBlockComponent.stories.tsx | 14 ----- dotcom-rendering/src/lib/renderElement.tsx | 2 - 8 files changed, 75 insertions(+), 83 deletions(-) diff --git a/dotcom-rendering/playwright/tests/parallel-5/atom.video.e2e.spec.ts b/dotcom-rendering/playwright/tests/parallel-5/atom.video.e2e.spec.ts index f0918e1d27d..cab8bd568d1 100644 --- a/dotcom-rendering/playwright/tests/parallel-5/atom.video.e2e.spec.ts +++ b/dotcom-rendering/playwright/tests/parallel-5/atom.video.e2e.spec.ts @@ -26,6 +26,7 @@ const interceptOphanPlayEvent = ({ page, id }: { page: Page; id: string }) => { .startsWith('http://ophan.theguardian.com/img/2?'); const searchParams = new URLSearchParams(request.url()); const videoSearchParam = searchParams.get('video'); + console.log(id, videoSearchParam); const expectedVideoSearchParam = JSON.stringify({ id, eventType: 'video:content:play', @@ -143,7 +144,7 @@ test.describe('YouTube Atom', () => { // Listen for the ophan call made when the video is played const ophanPlayEventPromise = interceptOphanPlayEvent({ page, - id: 'gu-video-youtube-2b33a7b7-e639-4232-9ecd-0fb920fa8147', + id: 'gu-video-youtube-1', }); // Listen for the YouTube embed call made when the video is played @@ -193,7 +194,7 @@ test.describe('YouTube Atom', () => { // Listen for the ophan call made when the video is played const ophanPlayEventPromise = interceptOphanPlayEvent({ page, - id: 'gu-video-youtube-2bc6f709-865e-49ae-b01b-8fc38eb4e9a7', + id: 'gu-video-youtube-1', }); // Listen for the YouTube embed call made when the video is played @@ -257,7 +258,7 @@ test.describe('YouTube Atom', () => { // Listen for the ophan call made when the video is played const ophanPlayEventPromise = interceptOphanPlayEvent({ page, - id: 'gu-video-youtube-1e89d5bd-489e-470a-857e-4f30e85b5aec', + id: 'gu-video-youtube-1', }); // Listen for the YouTube embed call made when the video is played @@ -297,7 +298,7 @@ test.describe('YouTube Atom', () => { // Listen for the ophan call made when the video is played const ophanPlayEventPromise2 = interceptOphanPlayEvent({ page, - id: 'gu-video-youtube-1e89d5bd-489e-470a-857e-4f30e85b5aec', + id: 'gu-video-youtube-2', }); // Listen for the YouTube embed call made when the video is played @@ -349,7 +350,7 @@ test.describe('YouTube Atom', () => { // Listen for the ophan call made when the video is played const ophanPlayEventPromise = interceptOphanPlayEvent({ page, - id: 'gu-video-youtube-2bc6f709-865e-49ae-b01b-8fc38eb4e9a7', + id: 'gu-video-youtube-1', }); // Listen for the YouTube embed call made when the video is played @@ -406,7 +407,7 @@ test.describe('YouTube Atom', () => { // Listen for the ophan call made when the video is played const ophanPlayEventPromise = interceptOphanPlayEvent({ page, - id: 'gu-video-youtube-1e89d5bd-489e-470a-857e-4f30e85b5aec', + id: 'gu-video-youtube-1', }); // Play main media video diff --git a/dotcom-rendering/src/components/Card/Card.tsx b/dotcom-rendering/src/components/Card/Card.tsx index af4e2f53b65..ab7fd3da09d 100644 --- a/dotcom-rendering/src/components/Card/Card.tsx +++ b/dotcom-rendering/src/components/Card/Card.tsx @@ -473,10 +473,6 @@ export const Card = ({ defer={{ until: 'visible' }} > { return (
{ return (
{
{
{
{
{
Scroll down...
{
Scroll down...
{ return (
{ />
{ return (
{ adTargeting={adTargeting} /> { adTargeting={adTargeting} /> {
Scroll down...
{ return (
{ return (
{ return (
{ return (
{ return (
{ return (
{
{
⬇️
{
⬇️
{ return (
{ />
{ return (
{ abTestParticipations={{}} /> { abTestParticipations={{}} /> { value={{ renderingTarget: 'Web', darkModeAvailable: false }} > { value={{ renderingTarget: 'Web', darkModeAvailable: false }} > { value={{ renderingTarget: 'Web', darkModeAvailable: false }} > { value={{ renderingTarget: 'Web', darkModeAvailable: false }} > { value={{ renderingTarget: 'Web', darkModeAvailable: false }} > { value={{ renderingTarget: 'Web', darkModeAvailable: false }} > { value={{ renderingTarget: 'Web', darkModeAvailable: false }} > { value={{ renderingTarget: 'Web', darkModeAvailable: false }} > { abTestParticipations={{}} /> (false); + useEffect(() => { + if (elementId === 'server') { + // We may never load the player on the server + return; + } + + if (!hasOverlay) { + // load the player if there is no overlay + setLoadPlayer(true); + } else if (overlayClicked) { + // load the player if the overlay has been clicked + setLoadPlayer(true); + } + }, [elementId, hasOverlay, overlayClicked]); /** * Create a stable callback as it will be a useEffect dependency in YoutubeAtomPlayer diff --git a/dotcom-rendering/src/components/YoutubeBlockComponent.importable.tsx b/dotcom-rendering/src/components/YoutubeBlockComponent.importable.tsx index c3527a6f2b8..24e15d13208 100644 --- a/dotcom-rendering/src/components/YoutubeBlockComponent.importable.tsx +++ b/dotcom-rendering/src/components/YoutubeBlockComponent.importable.tsx @@ -1,5 +1,6 @@ import { css } from '@emotion/react'; import type { ConsentState } from '@guardian/consent-management-platform/dist/types'; +import { isUndefined } from '@guardian/libs'; import { body, palette, space } from '@guardian/source-foundations'; import { SvgAlertRound } from '@guardian/source-react-components'; import { useEffect, useState } from 'react'; @@ -12,8 +13,6 @@ import { useConfig } from './ConfigContext'; import { YoutubeAtom } from './YoutubeAtom/YoutubeAtom'; type Props = { - id: string; - elementId: string; mediaTitle?: string; altText?: string; assetId: string; @@ -87,9 +86,9 @@ const getLargestImageSize = ( }[], ) => [...images].sort((a, b) => a.width - b.width).pop(); +let index: number | undefined; + export const YoutubeBlockComponent = ({ - id, - elementId, assetId, mediaTitle, altText, @@ -121,6 +120,13 @@ export const YoutubeBlockComponent = ({ abTestsApi?.isUserInVariant('IntegrateIma', 'variant') ?? false; const abTestParticipations = abTests?.participations ?? {}; + const [id, setId] = useState(); + + useEffect(() => { + index ??= 0; + setId(++index); + }, []); + useEffect(() => { const defineConsentState = async () => { const { onConsentChange } = await import( @@ -179,7 +185,7 @@ export const YoutubeBlockComponent = ({ } const ophanTracking = async (trackingEvent: string): Promise => { - if (!id) return; + if (isUndefined(id)) return; const ophan = await getOphan(renderingTarget); ophan.record({ video: { @@ -190,17 +196,17 @@ export const YoutubeBlockComponent = ({ }; const gaTracking = (trackingEvent: string) => { - if (!id) return; + if (isUndefined(id)) return; trackVideoInteraction({ trackingEvent, - elementId: id, + elementId: String(id), }); }; return (
{ }} assetId="d2Q5bXvEgMg" mediaTitle="Prince Harry and Meghan's 'bombshell' plans explained – video" - id="c2b8a51c-cb3d-41e7-bb79-1d9a091d0c28" - elementId="5ab531a2-f6ea-499d-b274-191114c8628c" expired={false} stickyVideos={false} /> @@ -82,8 +80,6 @@ export const Vertical = () => { }} assetId="d2Q5bXvEgMg" mediaTitle="Prince Harry and Meghan's 'bombshell' plans explained – video" - id="c2b8a51c-cb3d-41e7-bb79-1d9a091d0c28" - elementId="5ab531a2-f6ea-499d-b274-191114c8628c" expired={false} height={259} width={460} @@ -117,8 +113,6 @@ export const Expired = () => { }} assetId="d2Q5bXvEgMg" mediaTitle="Prince Harry and Meghan's 'bombshell' plans explained – video" - id="c2b8a51c-cb3d-41e7-bb79-1d9a091d0c28" - elementId="5ab531a2-f6ea-499d-b274-191114c8628c" expired={true} overrideImage="https://i.guim.co.uk/img/media/49565a29c6586fe6b748926e0be96c5e9c90473c/0_0_4981_2989/500.jpg?quality=85&auto=format&fit=max&s=17c70ec70002ea34886fd6c2605cd81e" height={259} @@ -153,8 +147,6 @@ export const WithOverlayImage = () => { }} assetId="d2Q5bXvEgMg" mediaTitle="Prince Harry and Meghan's 'bombshell' plans explained – video" - id="c2b8a51c-cb3d-41e7-bb79-1d9a091d0c28" - elementId="5ab531a2-f6ea-499d-b274-191114c8628c" expired={false} duration={333} overrideImage="https://i.guim.co.uk/img/media/49565a29c6586fe6b748926e0be96c5e9c90473c/0_0_4981_2989/500.jpg?quality=85&auto=format&fit=max&s=17c70ec70002ea34886fd6c2605cd81e" @@ -190,8 +182,6 @@ export const WithPosterImage = () => { }} assetId="d2Q5bXvEgMg" mediaTitle="Prince Harry and Meghan's 'bombshell' plans explained – video" - id="c2b8a51c-cb3d-41e7-bb79-1d9a091d0c28" - elementId="5ab531a2-f6ea-499d-b274-191114c8628c" expired={false} duration={333} posterImage={[ @@ -248,8 +238,6 @@ export const WithPosterAndOverlayImage = () => { }} assetId="d2Q5bXvEgMg" mediaTitle="Prince Harry and Meghan's 'bombshell' plans explained – video" - id="c2b8a51c-cb3d-41e7-bb79-1d9a091d0c28" - elementId="5ab531a2-f6ea-499d-b274-191114c8628c" expired={false} overrideImage="https://i.guim.co.uk/img/media/49565a29c6586fe6b748926e0be96c5e9c90473c/0_0_4981_2989/500.jpg?quality=85&auto=format&fit=max&s=17c70ec70002ea34886fd6c2605cd81e" duration={333} @@ -307,8 +295,6 @@ export const WithShowMainVideoFlagOff = () => { }} assetId="d2Q5bXvEgMg" mediaTitle="Prince Harry and Meghan's 'bombshell' plans explained – video" - id="c2b8a51c-cb3d-41e7-bb79-1d9a091d0c28" - elementId="5ab531a2-f6ea-499d-b274-191114c8628c" expired={false} overrideImage="https://i.guim.co.uk/img/media/49565a29c6586fe6b748926e0be96c5e9c90473c/0_0_4981_2989/500.jpg?quality=85&auto=format&fit=max&s=17c70ec70002ea34886fd6c2605cd81e" duration={333} diff --git a/dotcom-rendering/src/lib/renderElement.tsx b/dotcom-rendering/src/lib/renderElement.tsx index e9c328d3d59..87f07322f38 100644 --- a/dotcom-rendering/src/lib/renderElement.tsx +++ b/dotcom-rendering/src/lib/renderElement.tsx @@ -742,8 +742,6 @@ export const renderElement = ({ key={index} hideCaption={hideCaption} isMainMedia={isMainMedia} - id={element.id} - elementId={element.elementId} assetId={element.assetId} expired={element.expired} overrideImage={element.overrideImage}