Improve spacefinder's handling of ranked and list articles on mobile #1271
Conversation
🦋 Changeset detectedLatest commit: a6d166c 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 |
Jakeii
force-pushed
the
jlk/ads-mobile-ranked-lists
branch
from
e0ca18e
to
e05444b
Compare
Jakeii
added
the
[beta] @guardian/commercial
|
🚀 0.0.0-beta-20240215161943 published to npm as a beta release |
Jakeii
changed the title
|
I think this change could benefit from some adding some extra E2E testing. I'm concerned that other articles that use |
Jakeii
force-pushed
the
jlk/ads-mobile-ranked-lists
branch
from
dce30f8
to
3284694
Compare
E2E tests may be useful to ensure we don't break this functionality in future on articles we know about, but it won't tell us about the ones we don't know about! |
|
The best way to ensure these articles have ads where we expect I thought was to improve the article e2e tests to check the positions of the ads, which I've managed to do. However it's going to be quite laborious to update all these positions if intentional changes are ever made. I had a go writing a script to update the expected ad positions automatically, but I'm struggling to get playwright and typescript to play nicely together outside of the test (e.g. as a script), so abandoning that for now, I think limiting to 2 test articles will be fine for now? |
Jakeii
force-pushed
the
jlk/ads-mobile-ranked-lists
branch
from
da9d310
to
6b706e6
Compare
| @@ -290,19 +290,35 @@ const addDesktopInline2PlusAds = (): Promise<boolean> => { | |||
| const addMobileInlineAds = (): Promise<boolean> => { | |||
| const rules: SpacefinderRules = { | |||
| bodySelector: articleBodySelector, | |||
| slotSelector: ' > p', | |||
| candidateSelector: [ | |||
There was a problem hiding this comment.
This needs to be an array for multiple elements due to how the body and candidate selector is combined within spacefinder, if they were simply comma separated it would end up for example like .article-commercial-selector > p, h2, hr which would select h2 and hr elements that aren't direct children of the article body.
| minAboveSlot: 35, | ||
| minBelowSlot: 200, | ||
| // Usually we don't want an ad right before videos, embeds and atoms etc. so that we don't break up related content too much. But if we have a heading above, anything above the heading won't be related to the current content, so we can place an ad there. | ||
| bypassMinBelow: |
There was a problem hiding this comment.
Hopefully the comment makes sense but please let me know if it can be improved.
What does this change?
Sample articles
https://www.theguardian.com/football/2024/feb/09/premier-league-10-things-to-look-out-for-this-weekend
https://www.theguardian.com/culture/2024/feb/15/miski-omar-the-10-funniest-things-i-have-ever-seen-on-the-internet
https://www.theguardian.com/culture/2024/feb/08/say-it-with-a-kiss-the-20-greatest-smooches-on-film-ranked
Improve ad insertion around headings and
hrelementsOn these articles spacefinder is avoiding placing ads near anything that isn't a paragraph by quite a wide margin of 200px.
We have decided that we wouldn't mind ads appearing above headings, so we can add h2 elements to our candidate selectors, some articles also place hr's above these headings and others have media (images/videos/embeds) directly after that have rules to not have ads within 200px above.
This rule makes sense so that ads are not inserted between paragraphs related to the media and the media itself. However a heading element above would indicate that the content above the heading is not directly related to the media, so an ad above the heading would be fine.
After some iteration, the best way to handle it I thought was to allow headings to "bypass" the minBelow rule for an opponent. So that heading candidates are allowed to have an ad inserted above even if they are above a media element, other elements still need to be further away.
Handle candidates/opponents with negative margins
There was another issue with this because the headings have a negative top margin and the
hr's are only 1 pixel tall, so they were neither above or below it.Examples of new placement:
Screen.Recording.2024-02-15.at.16.10.43.mov
Screen.Recording.2024-02-16.at.10.20.32.mov
Why?
Otherwise there would be no ads on these articles on mobile and we can't have that.