-
-
Notifications
You must be signed in to change notification settings - Fork 79.1k
Docs: Prevent layout shift caused by Carbon ads #41452
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
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.
Thanks for taking a look and for the detailed breakdown of your approach — really appreciated.
I’ve noticed a couple of rendering issues:
Missing ad element causing layout shift:
When the ads don’t render (e.g. due to ad blockers), the #carbonads
element is absent, which leaves an unintended extra space:
One potential workaround might be something like (not tested):
.ad-container {
&:has(#carbonads) {
/* ... */
}
}
I'd prefer not using :has
but maybe there's no other approach to handle this use case.
Extra spacing above the ad:
There also seems to be additional spacing above the ad in some cases. Here’s a comparison showing the difference:


This seems to be due to .overflow-auto
somehow.
Apologies — I won’t be able to dive into this further until next week, as I’m away for a few days.
I agree the empty space with ad blockers is not great. Unfortunately I don't think this solution
No worries. In the meantime I'll look into the weird spacing issue and see if I can figure out a solution for the ad-blockers. |
The problem with the weird spacing is that sometimes the text of the ad is too long and makes the ad taller: Instead of reducing the font size, I've opted for increasing the height of the container by For the ad blockers the only solutions I could think of were:
Maybe 1 or 2 are acceptable? I can propose some solutions. I believe globally the share of users with ad blockers is less than 40%? Considering that, 200px of empty space seems like a good compromise and certainly better than the layout shift. That's just my personal opinion though. |
Thank you for your research and prototyping 🙏 At this point, your current solution seems to be the best fit for users who see the ads. For those who don’t see ads, I experimented with this idea 6411241, but had to revert it since it didn’t behave as expected when deployed on Netlify—the behavior differed from what I observed locally under a 3G throttle 🤷.
I’m not sure about the exact statistics, but we should focus on the majority of users, who likely see the ads. I can’t think of a solution that works well for both groups except possibly relocating the ads, though I’m unsure of the best place to move them. I'll get used to see this extra space 😄 If no one else has any ideas, maybe @mdo, we can go ahead and merge this patch (don't forget to squash the commits :) ) |
Thanks for all the effort here, sorry I didn't weigh in sooner! I opened #41506 to replace this and move the ads to the right sidebar. |
Description
As discussed in this issue with @julien-deramond , Carbon ads cause a layout shift in all docs pages. I also identified another layout shift in the home page.
I added a new
.ad-container
element in theAds.astro
component that reserves the space to prevent this shift usingmin-height
.Unfortunately, the CSS for the
.ad-container
element had to be duplicated in both_masthead.scss
and_layout.scss
for both the home page and the docs pages. It's common in Astro to include a<style>
tag to scope CSS to an Astro component but since this pattern is not used in this project I opted for duplicating the CSS. Other options would have been using inline styles or some global ID selector, which I also discarded.I also added a SCSS var so that the height can be changed in the future if Carbon ads change the height of their ads.
Motivation & Context
It's just ugly and annoying thing to have a layout shift that affects the whole content of all the docs pages.
Type of changes
Checklist
npm run lint
)Live previews
Related issues
Closes #41448