Show ads on mobile discussion #1239

deedeeh · 2024-02-01T11:45:27Z

What does this change?

This PR is an OKR ticket to insert MPU ads in mobile discussion as the discussion readers are our most engaged readers. Other PRs linked to this one guardian/dotcom-rendering#10448 and guardian/frontend#26899

The first ad is shown after the 4th comment and then an ad is inserted after every 5 comments. We don't insert every 5 comments from the beginning because the number of comments is usually a multiple of 5, and showing an ad after the last comment doesn't look right from a UX point of view.

The code listens for the custom event comments-loaded from DCR and we insert comments-expanded-${id} ads.

The code also listens for the comments-state-change event which is fired when the user makes a change in filter or to the page number. When we receive the comments-state-change event we remove the previously inserted comments-expanded adverts. We make sure we are on mobile breakpoint so we don't remove the comments-expanded ads on the right column on desktop view.

This feature is behind a feature switch for now.

Why?

Increase revenue and ad ratio without impacting the user experience.

Demo

mobile-expanded-comments-480.mov

changeset-bot · 2024-02-01T11:45:31Z

🦋 Changeset detected

Latest commit: c061891

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@guardian/commercial	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-02-12T16:09:51Z

🚀 0.0.0-beta-20240212160939 published to npm as a beta release

github-actions · 2024-02-13T12:07:10Z

🚀 0.0.0-beta-20240213120659 published to npm as a beta release

src/core/ad-sizes.ts

arelra

📝

arelra · 2024-02-14T12:57:34Z

src/insert/comments-expanded-advert.ts

+const getCommentsColumn = async (): Promise<HTMLElement> => {
+	return fastdom.measure(() => {
+		const commentsColumn: HTMLElement | null =
+			document.querySelector('.comments-column');


This might break if the upstream class naming changes.

Would an e2e test be beneficial for this functionality? We could check that ads are inserted into mobile discussion, which would also cover if this contract breaks.

Yeah good point! We added the class name ourselves in DCR, we can add a comment to the code to explain to other developers why it was added to help discourage deletion. An e2e is test is a good idea - we'll have a look at implementing that

Would using a data attribute be more appropriate? We use data attributes for spacefinder: https://github.com/guardian/dotcom-rendering/blob/75379021a041660acee798cd3ed8902775bf14cf/dotcom-rendering/docs/contracts/001-commercial-selectors.md

I wonder if we should save writing an e2e test until we have the AB test results? We don't know for sure that this feature will be implemented yet. Would definitely be a valuable addition if we do implement the feature though!

domlander · 2024-02-14T13:00:05Z

Can space be reserved for the ad once the ad slot is inserted into the DOM? This will prevent CLS

domlander · 2024-02-14T14:36:09Z

Not an issue, just something to think about:

It is possible that a comments-expanded ad can appear at the bottom of the comments section when it is expanded. This is fine in the below image as it sits above the Most Viewed container. Will the Most Viewed container always directly below the comments? Are there any situation where a different type of advert could be below the comments container?

Jakeii · 2024-02-14T14:45:11Z

You may have already explained this, but what made you decide to add the slots via JS, rather than in the react code in DCR?

emma-imber · 2024-02-14T14:54:52Z

Not an issue, just something to think about:

It is possible that a comments-expanded ad can appear at the bottom of the comments section when it is expanded. This is fine in the below image as it sits above the Most Viewed container. Will the Most Viewed container always directly below the comments? Are there any situation where a different type of advert could be below the comments container?

The only case in DCR where the Most Viewed component doesn't appear below the Discussion Component is when the content is paid for. I may be wrong but I've never seen comments on paid for content?

emma-imber · 2024-02-14T14:55:57Z

You may have already explained this, but what made you decide to add the slots via JS, rather than in the react code in DCR?

It was simply that it made sense to us for all of the comments-expanded logic to live together in one file.

domlander · 2024-02-14T15:09:02Z

src/insert/comments-expanded-advert.ts

@@ -67,6 +106,30 @@ const createResizeObserver = (rightColumnNode: HTMLElement) => {
 	resizeObserver.observe(rightColumnNode);
 };

+const removeMobileCommentsExpandedAds = (): Promise<void> => {
+	const currentBreakpoint = getBreakpoint(getViewport().width);
+	if (currentBreakpoint === 'mobile') {


Could we include tablet as well? We don't show a right column until the desktop breakpoint. Happy for this to be a follow-up ticket.

The ticket just specified mobile for this work which is why we haven't implemented tablet. I think it makes more sense as a follow-up ticket

domlander · 2024-02-14T15:19:07Z

src/insert/comments-expanded-advert.ts

@@ -53,6 +89,9 @@ const isEnoughSpaceForAd = (rightColumnNode: HTMLElement): boolean => {
 	return rightColumnNode.offsetHeight >= minHeightToPlaceAd;
 };

+const isEnoughCommentsForAd = (commentsColumn: HTMLElement): boolean =>
+	commentsColumn.childElementCount > 5;


If we insert an advert after every fourth comment and it's acceptable for an advert to be at the bottom of the comments, should this be commentsColumn.childElementCount >= 4;?

We've checked this with Alex and have decided that an advert should never be at the bottom of the comments. I've introduced a check to make sure that this doesn't happen.

Sounds good. This will also come in useful when we add comments to the frontend crossword comments, where the merchandising-high slot follows the comments.

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Co-authored-by: Dina Hafez <dina.hafez@guardian.co.uk>

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

github-actions · 2024-02-20T11:53:07Z

🚀 0.0.0-beta-20240220115252 published to npm as a beta release

deedeeh changed the title ~~Show ads on mobile discussion~~ Show ads on mobile discussion AB tests Feb 1, 2024

deedeeh force-pushed the dina/show-ads-on-mobile-discussion branch from 606acc9 to 09dc6f0 Compare February 1, 2024 16:07

emma-imber force-pushed the dina/show-ads-on-mobile-discussion branch 2 times, most recently from eebc53f to d007a78 Compare February 12, 2024 15:29

deedeeh added [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR and removed [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR labels Feb 12, 2024

deedeeh added [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR and removed [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR labels Feb 13, 2024

deedeeh removed the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Feb 13, 2024

This was referenced Feb 13, 2024

Show ads on mobile discussion guardian/dotcom-rendering#10448

Merged

Show ads on mobile discussion guardian/frontend#26899

Merged

deedeeh assigned deedeeh and emma-imber Feb 13, 2024

emma-imber marked this pull request as ready for review February 14, 2024 09:48

emma-imber requested a review from a team as a code owner February 14, 2024 09:48

domlander reviewed Feb 14, 2024

View reviewed changes

src/core/ad-sizes.ts Outdated Show resolved Hide resolved

arelra approved these changes Feb 14, 2024

View reviewed changes

domlander reviewed Feb 14, 2024

View reviewed changes

deedeeh and others added 2 commits February 20, 2024 11:42

Insert ad into comments

2a3a724

Insert unique ids for comments-expanded mobile ads

57c0b40

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

emma-imber and others added 20 commits February 20, 2024 11:42

Fix imports

581bc84

Use setTimeout to load elements in the DOM

363ce6d

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Added a function that removes dynamically inserted ads in comments

b7acead

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Remove existing slots on page change custom event

4f06b44

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Remove ad slots from dfp

d942ca8

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Resize label and center discussion ads

d8ed1f2

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Add destroySlots and timeout to wait for updated ul element

2191871

Separate and tidy logic

79f7660

Co-authored-by: Dina Hafez <dina.hafez@guardian.co.uk>

Post rebase tidy up

e90be8a

Don't use fastdom for getCommentsColumn()

dc147c6

Remove unnecessary void

7e428b1

Get comments-expanded ads working after rebase

b67513d

Remove setTimeout as no longer needed

c3e5626

Co-authored-by: Dina Hafez <dina.hafez@guardian.co.uk>

Check if user is in variant

ab40cb6

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Add changeset

26ade80

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Add comments-expanded to slot config

46c83e5

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Reserve space for the mobile comments expanded

18ae735

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

Add check so that comments section never ends in an ad

c88686b

Don't insert slots if there are already some present on the page

05629a0

Replace comments-column class with data-attribute

ac3d20e

Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>

emma-imber force-pushed the dina/show-ads-on-mobile-discussion branch from 5609c44 to ac3d20e Compare February 20, 2024 11:43

emma-imber added 2 commits February 20, 2024 11:44

Add adSizes.empty to mobile comments-expanded slot

1c7e25b

Use switch instead of AB test

c061891

emma-imber added the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Feb 20, 2024

emma-imber changed the title ~~Show ads on mobile discussion AB tests~~ Show ads on mobile discussion Feb 20, 2024

deedeeh merged commit d285016 into main Feb 21, 2024
12 checks passed

deedeeh deleted the dina/show-ads-on-mobile-discussion branch February 21, 2024 10:36

emma-imber mentioned this pull request Mar 13, 2024

Add Playwright test for comments-expanded ads #1302

Merged

Jakeii mentioned this pull request Apr 2, 2024

Commercial Q4 mega test #1318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show ads on mobile discussion #1239

Show ads on mobile discussion #1239

deedeeh commented Feb 1, 2024 •

edited by emma-imber

Loading

changeset-bot bot commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024

github-actions bot commented Feb 13, 2024

arelra left a comment

arelra Feb 14, 2024

emma-imber Feb 14, 2024

domlander Feb 14, 2024

emma-imber Feb 14, 2024

domlander commented Feb 14, 2024

domlander commented Feb 14, 2024 •

edited

Loading

Jakeii commented Feb 14, 2024

emma-imber commented Feb 14, 2024 •

edited

Loading

emma-imber commented Feb 14, 2024

domlander Feb 14, 2024

emma-imber Feb 14, 2024

domlander Feb 14, 2024 •

edited

Loading

emma-imber Feb 14, 2024

domlander Feb 14, 2024

github-actions bot commented Feb 20, 2024

Show ads on mobile discussion #1239

Show ads on mobile discussion #1239

Conversation

deedeeh commented Feb 1, 2024 • edited by emma-imber Loading

What does this change?

Why?

Demo

changeset-bot bot commented Feb 1, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 12, 2024

github-actions bot commented Feb 13, 2024

arelra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domlander commented Feb 14, 2024

domlander commented Feb 14, 2024 • edited Loading

Jakeii commented Feb 14, 2024

emma-imber commented Feb 14, 2024 • edited Loading

emma-imber commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domlander Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2024

deedeeh commented Feb 1, 2024 •

edited by emma-imber

Loading

changeset-bot bot commented Feb 1, 2024 •

edited

Loading

domlander commented Feb 14, 2024 •

edited

Loading

emma-imber commented Feb 14, 2024 •

edited

Loading

domlander Feb 14, 2024 •

edited

Loading