Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add mobile banner below crossword controls static with AB test #1229

Merged
merged 13 commits into from
Feb 8, 2024

Conversation

dskamiotis
Copy link
Contributor

@dskamiotis dskamiotis commented Jan 25, 2024

What does this change?

This PR adds a stationary mobile-banner ad-slot to crosswords article. It is positioned just below the controls and rendered via the commercial bundle.

A variety of logic has been added to ensure this renders the ad appropriately, by looking for an element as an anchor in which to add a container wit had slot, which is set in frontend.

The crossword may render before the commercial code loads and we do not want to introduce CLS with this change.
Logic has been added to mitigate against this with an listener.

This is in conjunction with the related frontend PR here

Why?

This is one of 2 options for different positions of adding ad-slot banner in crossword pages.
We would like to test, each scenario and analyse impact.
This has been set u to be a server-side test, and will be i a 2% bucket for the control and variant.

Screenshot 2024-01-25 at 16 55 35

Tested on CODE with frontend

Copy link

changeset-bot bot commented Jan 25, 2024

🦋 Changeset detected

Latest commit: fddb78c

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 Patch

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

@dskamiotis dskamiotis marked this pull request as ready for review January 25, 2024 17:00
@dskamiotis dskamiotis requested a review from a team as a code owner January 25, 2024 17:00
@SiAdcock
Copy link
Contributor

Thanks for your work on this @dskamiotis 🙏

Shall we think about naming for this new slot? I think crossword-banner or mobile-banner make more sense than mobile-sticky (other opinions appreciated!), so I would reflect that in the filename for this module.

@domlander
Copy link
Contributor

Thanks for your work on this @dskamiotis 🙏

Shall we think about naming for this new slot? I think crossword-banner or mobile-banner make more sense than mobile-sticky (other opinions appreciated!), so I would reflect that in the filename for this module.

I like crossword-banner, as this ad slot will only be used on the crosswords page. I don't think the new slot should contain sticky, as the ad slot may not be sticky (despite the ad size being called mobile-sticky).

@SiAdcock
Copy link
Contributor

SiAdcock commented Jan 29, 2024

@domlander

I like crossword-banner, as this ad slot will only be used on the crosswords page.

I liked it too, until @Amouzle pointed out that it already exists, for a leaderboard ad that appears below the grid at phablet width 😅

@emma-imber
Copy link
Contributor

@domlander

I like crossword-banner, as this ad slot will only be used on the crosswords page.

I liked it too, until @Amouzle pointed out that it already exists, for a leaderboard ad that appears below the grid at phablet width 😅

Yep was going to jump in to say this! It's this one for anyone who's not seen it before:

Screenshot 2024-01-29 at 16 25 37

Perhaps we could rename it and have crossword-banner-mobile and crossword-banner-tablet or something similar?

@domlander
Copy link
Contributor

domlander commented Jan 29, 2024

@domlander

I like crossword-banner, as this ad slot will only be used on the crosswords page.

I liked it too, until @Amouzle pointed out that it already exists, for a leaderboard ad that appears below the grid at phablet width 😅

Perhaps we could rename it and have crossword-banner-mobile and crossword-banner-tablet or something similar?

I wonder if there are good reasons why we shouldn't reuse the slot name? The mobile ad slot will never appear on tablet and vice versa.

@dskamiotis
Copy link
Contributor Author

@domlander

I like crossword-banner, as this ad slot will only be used on the crosswords page.

I liked it too, until @Amouzle pointed out that it already exists, for a leaderboard ad that appears below the grid at phablet width 😅

Perhaps we could rename it and have crossword-banner-mobile and crossword-banner-tablet or something similar?

I wonder if there are good reasons why we shouldn't reuse the slot name? The mobile ad slot will never appear on tablet and vice versa.

As its a separate module, assuming its isolated in execution? Therefore the same naming will not have have any conflicts.

@dskamiotis
Copy link
Contributor Author

@domlander

I like crossword-banner, as this ad slot will only be used on the crosswords page.

I liked it too, until @Amouzle pointed out that it already exists, for a leaderboard ad that appears below the grid at phablet width 😅

Yep was going to jump in to say this! It's this one for anyone who's not seen it before:

Screenshot 2024-01-29 at 16 25 37 Perhaps we could rename it and have `crossword-banner-mobile` and `crossword-banner-tablet` or something similar?

Yes I think suggestion crossword-banner-mobile is most appropriate as and exclusive to mobile as an new ad slot size, as it will never been seen on tablet?

@dskamiotis dskamiotis force-pushed the ds/add-mobile-ad-crosswords branch 2 times, most recently from 709b405 to 2f7a8e2 Compare February 5, 2024 17:23
@dskamiotis dskamiotis added the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Feb 7, 2024
Copy link
Contributor

github-actions bot commented Feb 7, 2024

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

@dskamiotis dskamiotis changed the title add mobile sticky below crossword controls static add mobile banner below crossword controls static with AB test Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants