-
Notifications
You must be signed in to change notification settings - Fork 20
Reintroduce the stickybox on wide screens #604
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
Reintroduce the stickybox on wide screens #604
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I tested it locally and it worked great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
@@ -198,11 +198,22 @@ export class EthicalAdsAddon extends AddonBase { | |||
if (elementToAppend) { | |||
elementToAppend.append(placement); | |||
} | |||
} else if (window.innerWidth > 1300 && Math.random() > 0.5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be another place to eventually use media query events, so that we can re-evaluate this logic if the view port size changes. This will just execute once on page load.
Noting for later, I don't think we are using this type of logic anywhere else yet.
@@ -198,11 +198,22 @@ export class EthicalAdsAddon extends AddonBase { | |||
if (elementToAppend) { | |||
elementToAppend.append(placement); | |||
} | |||
} else if (window.innerWidth > 1300 && Math.random() > 0.5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid the randomness here. I think a better approach is using the same logic for each user, so it doesn't change randomly on refresh.
Will merge this for release today. |
Did a simple version here: #607 |
This reverts commit bc62388.
Alternates between using the stickybox placement and the footer placement on unknown themes. It also adds an ID to the stickybox placement so we can see performance of it.
As written, this introduces a random element which may not be desirable. We could instead introduce the stickybox only on very wide screens (say >=1600px) or we could base it on the title of the project or something like that to make it deterministic. I'm open to suggestions.
The footer placement is performing very well at ~4x the CTR with ~50% the view rate vs. previously.
Ref: #562