-
Notifications
You must be signed in to change notification settings - Fork 0
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
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.
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:
- https://developer.mozilla.org/en-US/docs/Web/CSS/aspect-ratio
- https://jakearchibald.com/2022/img-aspect-ratio/
- https://css-tricks.com/almanac/properties/a/aspect-ratio/
@jurgenbelien do you agree?
src/blocks/ImageBlock/Image.astro
Outdated
@@ -37,6 +37,8 @@ const imageUnavailableMessage = t('image_unavailable'); | |||
loading={ loading } | |||
srcset={ getImageSrcSet(image.url) } | |||
src={ getImageSrc(image.url) } | |||
height={image.height} |
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.
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.
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.
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. 🤷♂️
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.
And I dont know if this is actually a problem since 1.5659 / 1 is kind of the same as 1.5659
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.
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.
Deploying head-start with Cloudflare Pages
|
f615812
to
be8c674
Compare
be8c674
to
76a0d3d
Compare
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.
Approved, we discussed this and leave the width
and height
in but remove the aspectRatio
.
Changes
Associated issue
Issue seen and fixed in Happy Labs.
How to test
ignore cache
and set network throttling onChecklist
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