Skip to content

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

Closed
wants to merge 10 commits into from

Conversation

PierBover
Copy link

@PierBover PierBover commented May 7, 2025

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 the Ads.astro component that reserves the space to prevent this shift using min-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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

Closes #41448

Copy link
Member

@julien-deramond julien-deramond left a 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:

Screenshot 2025-05-07 at 19 06 13

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:

Screenshot 2025-05-07 at 19 08 30 Screenshot 2025-05-07 at 19 08 25

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.

@julien-deramond julien-deramond changed the title fix layout shift caused by carbon ads Docs: Prevent layout shift caused by Carbon ads May 7, 2025
@PierBover
Copy link
Author

I agree the empty space with ad blockers is not great. Unfortunately I don't think this solution &:has(#carbonads) will fix it since the #carbonads element is not present until it's added via JS (so the space won't be reserved until JS has loaded the ad).

I won’t be able to dive into this further until next week, as I’m away for a few days.

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.

@PierBover
Copy link
Author

PierBover commented May 8, 2025

The problem with the weird spacing is that sometimes the text of the ad is too long and makes the ad taller:

image

Instead of reducing the font size, I've opted for increasing the height of the container by 6px.

For the ad blockers the only solutions I could think of were:

  1. Placing the ad with absolute positioning somewhere else so that the layout shift won't happen.

  2. Moving the ad somewhere else but causing a less obvious layout shift. Right now it moves pretty much all the main content of the page.

  3. Block rendering with JS and somehow detect if there's an ad blocker and then prevent the layout shift somehow...

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.

@github-project-automation github-project-automation bot moved this to To do in v5.3.7 May 22, 2025
@julien-deramond julien-deramond moved this from To do to Needs review in v5.3.7 May 22, 2025
@julien-deramond
Copy link
Member

julien-deramond commented May 23, 2025

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 believe globally the share of users with ad blockers is less than 40%?

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 :) )

@julien-deramond julien-deramond requested a review from mdo May 23, 2025 20:42
@julien-deramond julien-deramond moved this from Needs review to Review in progress in v5.3.7 May 24, 2025
@mdo mdo removed this from v5.3.7 May 29, 2025
@mdo
Copy link
Member

mdo commented May 29, 2025

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.

@mdo mdo closed this May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Prevent layout shift caused by Carbon ads
3 participants