-
Notifications
You must be signed in to change notification settings - Fork 29
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
Show ads on mobile discussion #10448
Conversation
Size Change: +1.01 kB (0%) Total Size: 763 kB
ℹ️ View Unchanged
|
708d6f7
to
3963649
Compare
0bf24b9
to
189ad25
Compare
27c337b
to
bd1bb9c
Compare
Hello 👋! When you're ready to run Chromatic, please apply the |
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 think it would be preferable to have the slots inserted by inside this component directly.
dotcom-rendering/src/experiments/tests/mobile-discussion-ads.ts
Outdated
Show resolved
Hide resolved
dotcom-rendering/src/experiments/tests/mobile-discussion-ads.ts
Outdated
Show resolved
Hide resolved
I believe we're planning to reuse this logic to make a similar change in frontend. From a maintainability perspective, it makes sense to me to keep this reusable logic in commercial, instead of having it defined one way in DCR, and then in a different way for frontend. |
d23bde6
to
423508f
Compare
db309c2
to
3d20ee0
Compare
2d04bdc
to
5ef5932
Compare
c6fc227
to
b0ff7fa
Compare
Co-authored-by: Dina Hafez <dina.hafez@guardian.co.uk>
Co-authored-by: Dina Hafez <dina.hafez@guardian.co.uk>
Co-authored-by: Dina Hafez <dina.hafez@guardian.co.uk>
Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>
Co-authored-by: Dominik Lander <dominik.lander@guardian.co.uk>
Co-authored-by: Max Duval <max.duval@theguardian.com>
Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>
Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>
Co-authored-by: Emma Imber <108270776+emma-imber@users.noreply.github.com>
b0ff7fa
to
45dc6b8
Compare
} | ||
}, [isWeb, expanded, loading, enableMobileDiscussionAdsSwitch]); | ||
|
||
const commentsStateChangeEvent = new CustomEvent('comments-state-change'); |
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.
Does this need to be defined within the component’s body?
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.
A foreboding comment!
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.
We should add a test for DiscussionWeb
in Island.test.tsx
How long is the |
Yes we don't need it long-term, a week or so, just to give us the ability to switch it off quickly if we need to. |
What does this change?
This PR is part of Commercial OKR work. We're inserting MPU ads in mobile discussion behind a switch. Other PRs linked to this one guardian/commercial#1239 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.
When the comments are expanded and are finished loading we dispatch a custom event
comments-loaded
to Commercial. The Commercial code handles this event and inserts ads.When the user makes any changes to the filter or change the comments page number we dispatch the
comments-state-change
event which removes the previous ad slots.Comments in apps will be served via DCR, which is a work in progress in WebX, so we added a check to insert ads if we are only in Web.
Why?
Increase revenue and ad ratio without impacting the user experience.
Demo
mobile-expanded-comments-480.mov