fix: add mutation observer to hide ads with unfilled status #41
Conversation
| if (el) { | ||
| const observer = new MutationObserver(() => { | ||
| if (el.getAttribute('data-ad-status') === 'unfilled') { | ||
| setIsHidden(true); |
There was a problem hiding this comment.
The mutation observer is not disconnected after changing the state, which might lead to a memory leak. Consider disconnecting the observer after it has fulfilled its purpose.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Thanks for the pull request!
According to this Google help page, care should be taken when hiding unfilled ad slots, as that can cause layout shifts - and these can be more disruptive than leaving the space unfilled.
With the current implementation, setIsHidden(true) uses conditional rendering to entirely skip rendering the <div> element containing the ad. This works for the mid-page horizontal ads (despite the layout shift), but as for the other slots, not all of them can be hidden equally:
- The container holding the side rail ad slots and the main content is a flex box, so the space occupied by the side rails cannot be hidden without the center column reflowing to its side. As such, it has to use
visibility: hiddeninstead ofdisplay: none.
- The song card ads cannot be hidden either, as that would cause all subsequent songs to collapse into its position. The ideal solution would be to display something in its place instead - or make sure they're always spawned at the last slot so this reflow doesn't occur.
Although it's possible to patch the PR to handle these cases, I believe more careful planning is required around the current ad placement to avoid the impact of unfilled ads. If they're getting unfilled often, it's likely that we're placing too many, so a better fix would be a more mindful placement (e.g. not all pages of the recent songs section need to have one).
No description provided.