TopRightAdSlot - #3968
Merged
Merged
TopRightAdSlot#3968
TopRightAdSlot#3968Conversation
oliverlloyd
requested review from
a team,
JamieB-gu and
marsavar
as code owners
February 11, 2022 11:07
|
Size Change: -36.3 kB (-3%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Contributor
|
I think we want to always render the ad slot. We conditionally also add the shady pie.
Not rendering this ad slot server side may have other consequences. I think this would be preferable to an approach where the slot is rendered at some unknown time. Pinging @guardian/commercial-dev for visibility. |
sndrs
approved these changes
Feb 11, 2022
sndrs
previously approved these changes
Feb 11, 2022
sndrs
dismissed
their stale review
February 11, 2022 11:54
should have spotted max's comment, i was too premature
Contributor
Author
As discussed, we do. The default top right as slot is always rendered server side and only replaced with shady pie on the client in the event that we detect an adblocker |
mxdvl
reviewed
Feb 11, 2022
mxdvl
left a comment
Contributor
There was a problem hiding this comment.
Okay, I get it now: the ad slot is rendered on the server, then hydrated on the client. There’s no risk of it being missing by the time commercial code runs.
mxdvl
approved these changes
Feb 11, 2022
Contributor
|
🏝️ #3629 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change?
This PR moves shady pie into an island
What is shady pie?
This is the name given to the contribution slot which gets shown in the top right ad slot when an ad blocker is detected (plus some other criteria).
In this PR we're creating a pure
ShadyPiecomponent which encapsulates what gets show. We then moved the logic to decide when to show it or not intoTopRightAdSlot. (I really wanted to call this componentMaybeShadybut in truth what it is actually doing is rendering the top right ad slot and then sometime this slot is the contribution - shady pie - image.)But now we have ad slot html in two different files, boo
I agree, it's not ideal that this html exists in multiple locations as this will make refactors harder later. I did try using
childrento keep theid="dfp-ad--right"div co-located inAdSlot.tsxhowever this introduced an unwanted side effect. The whole slot html and css was being serialised and inserted into thegu-islanddom element. This is sub optimal and I felt that the trade off between this performance hit and the fact the code is broken apart was worth it.