Skip to content
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

fix: avoid layout shift due to ads #176

Merged
merged 3 commits into from
Jan 22, 2021
Merged

fix: avoid layout shift due to ads #176

merged 3 commits into from
Jan 22, 2021

Conversation

posva
Copy link
Member

@posva posva commented Dec 4, 2020

This is specific to mobile viewports. Since the ad container goes on top of the content, loading it only on client side creates a layout shift:

Screen-Recording-2020-12-04-at-11 25 56

@vercel
Copy link

vercel bot commented Dec 4, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vuejs/vitepress/ii4y0zkjb
✅ Preview: https://vitepress-git-fix-layout-shift-ads.vuejs1.vercel.app

@posva
Copy link
Member Author

posva commented Dec 4, 2020

I think this is the simplest and most maintainable solution: remove the async loading for the CarbonAds component + min-height on the ads container. It does add the CarbonAds to the whole bundle but I would say it's reasonable.

We can also expose the Component to be used by custom themes or build a more complex solution like a placeholder that takes up the space on mobile, which would create duplication.

@posva
Copy link
Member Author

posva commented Dec 5, 2020

Should be good with a container in layout and keeping the component async!

@kiaking kiaking merged commit 78b026c into master Jan 22, 2021
@kiaking kiaking deleted the fix/layout-shift-ads branch January 22, 2021 15:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants