Create advert density by section AB test #1296
Conversation
🦋 Changeset detectedLatest commit: 6f4c2a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
domlander
force-pushed
the
doml/section-ad-density-test
branch
from
f63decf
to
bf6f92f
Compare
| @@ -201,7 +201,6 @@ const countLiveblogInlineSlots = async (page: Page, isMobile: boolean) => { | |||
| return await page.locator(locator).count(); | |||
| }; | |||
|
|
|||
| // @ts-expect-error -- used when logging uncommented | |||
There was a problem hiding this comment.
We get a warning on every PR with this unused function. Can we leave the console.warn in? It's only used for Playwright.
| @@ -147,9 +151,10 @@ const addDesktopInline1 = (): Promise<boolean> => { | |||
| minAbove: isImmersive ? 700 : 300, | |||
| minBelow: 300, | |||
| opponentSelectorRules: { | |||
| // don't place ads right after a heading | |||
There was a problem hiding this comment.
This seemed a little contradictory before: We allow an ad >5px below a heading but we must keep >190px above a heading? We shouldn't put ads closely below heading as this is likely to break up semantically related content. I think this was perhaps a mistake given the confusing naming of minAboveSlot .
| @@ -288,14 +292,16 @@ const addDesktopInline2PlusAds = (): Promise<boolean> => { | |||
| }; | |||
|
|
|||
| const addMobileInlineAds = (): Promise<boolean> => { | |||
| const minDistanceFromArticleTop = isInAdDensityVariant ? 100 : 200; | |||
There was a problem hiding this comment.
From testing on my real device (N=1), this ad is still below the fold by a fair way, so I think this should be OK.
| // selector(s) for the elements that we want to allow inserting ads above | ||
| /** | ||
| * Selector(s) for the elements that we want to allow inserting ads above | ||
| */ | ||
| candidateSelector: string | string[]; |
There was a problem hiding this comment.
This is so that the VS Code feature of hovering over a property to see its description works.
Jakeii
added
the
[beta] @guardian/commercial
|
🚀 0.0.0-beta-20240312170232 published to npm as a beta release |
What does this change?
Fix a bug with spacefinder not use the expected
minAboveSlotwhen avoiding other winners on mobile, it adds 250px (the height of an MPU).This was because spacefinder was using the same approach for floated right ads and inline ads. The additions to the docs explains why different approaches are needed for each type of ad.
So as not to change the ad density everywhere, I've fixed the bug, and changed the margin to 750px for mobile and retained 500px for desktop.
This PR also adds an AB test that will increase ad-density on articles in specific sections of the Guardian, e.g. business, money. For these test "high value sections" we will reduce it to
500pxon mobile.And finally fixes a bug with spacefinder avoiding elements inserted by the spacefinder debugger
Why?