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: Layout shifts caused by images #190

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

luukbrauckmann
Copy link
Member

@luukbrauckmann luukbrauckmann commented Oct 2, 2024

Changes

  • Fixes layout shifts by setting width and height on images

Associated issue

Issue seen and fixed in Happy Labs.

How to test

  1. Open preview link in English
  2. Open inspector
  3. Go to network tab
  4. Enable ignore cache and set network throttling on
  5. You'll see the image size being reserved.

Checklist

  • I have performed a self-review of my own code
  • I have made sure that my PR is easy to review (not too big, includes comments)
  • I have made updated relevant documentation files (in project README, docs/, etc)
  • I have added a decision log entry if the change affects the architecture or changes a significant technology
  • I have notified a reviewer

@luukbrauckmann luukbrauckmann self-assigned this Oct 2, 2024
@luukbrauckmann luukbrauckmann marked this pull request as ready for review October 2, 2024 08:52
Copy link
Member

@jbmoelker jbmoelker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at different articles to better understand why this issue occurs. Either because an img is a "replaced element", or because the display is incorrect or because the dimensions are over constrained or because the browser can't make out the intrinsic dimensions because it contains a srcset, but that shouldn't matter with a specified aspect ratio. I couldn't find anything that explains the cause of our issue. I do see that your change solves it. So I'm inclined to approve and accept it :)

Resources I checked:

@jurgenbelien do you agree?

@@ -37,6 +37,8 @@ const imageUnavailableMessage = t('image_unavailable');
loading={ loading }
srcset={ getImageSrcSet(image.url) }
src={ getImageSrc(image.url) }
height={image.height}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solves the same issue as the aspect ratio inline styling on line 43. So one or the other should be enough. I did notice that the inline calculation is actually calculated instead of dumped as a string, so thats probably why there is reflow with aspect ratio in place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok just to be clear, on page load the value of the aspect ratio is not correct, but when javascript kicks in it will be set correctly, which is too late ofcourse. No idea why JS has influence on this but on page https://head-start.pages.dev/en/demos/text-image-block/ the first image first has an aspect ratio of aspect-ratio:1.5658959537572255; and after JS kicks in it has a value of aspect-ratio: 1.5659 / 1;. You can test it with JS disabled. 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I dont know if this is actually a problem since 1.5659 / 1 is kind of the same as 1.5659

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soooo, the problem seems to be the absence of width 100% on the image in combination with aspect-ratio. The aspect ratio works if you add width 100% to the image.

Copy link

cloudflare-workers-and-pages bot commented Oct 18, 2024

Deploying head-start with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4fa5c14
Status: ✅  Deploy successful!
Preview URL: https://c0ff9098.head-start.pages.dev
Branch Preview URL: https://fix-layout-shifts-caused-by.head-start.pages.dev

View logs

@luukbrauckmann luukbrauckmann force-pushed the fix/layout-shifts-caused-by-images branch from f615812 to be8c674 Compare October 18, 2024 09:01
@luukbrauckmann luukbrauckmann force-pushed the fix/layout-shifts-caused-by-images branch from be8c674 to 76a0d3d Compare October 18, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants