-
Notifications
You must be signed in to change notification settings - Fork 77
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
First batch of images picture tag #377
Conversation
NeOMakinG
commented
Oct 17, 2022
•
edited
Loading
edited
Questions | Answers |
---|---|
Description? | We wants to switch images to picture tags and support webp and avif for 8.1 |
Type? | improvement |
BC breaks? | no |
Deprecations? | no |
How to test? | Images should work as expected, double density screens should receive 2x sizes |
Possible impacts? | Images |
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.
Quick check
src="{$image.bySize.default_720.sources.jpg}" | ||
width="{$image.bySize.default_720.width}" | ||
height="{$image.bySize.default_720.height}" | ||
loading="lazy" |
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.
We need to use loading="eager"
on the first image. It will be FCP element that we need to load ASAP. It would maybe be also good to add a preload element to header so it starts loading right away.
Something like:
<link rel="preload" as="image"
imagesrcset="{$product.cover.bySize.medium_default.url} 452w,
{$product.cover.bySize.pdt_180.url} 180w,
{$product.cover.bySize.pdt_300.url} 300w,
{$product.cover.bySize.pdt_360.url} 360w,
{$product.cover.bySize.pdt_540.url} 540w,
{$product.cover.bySize.pdt_590.url} 590w"
imagesizes="(min-width: 1300px) 590px, (min-width: 768px) 50vw, 100vw"
href="{$product.cover.bySize.medium_default.url}">
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.
Leaving the comment opened for the preload if we want to implement it
I'm not sure this is the right way. IMO names should be specific to the target: |
@SharakPL Well, the point is, that the images are usable on multiple places sometime. Our notes are something like this:
|
Of course some common sizes could be kept eg.
And what's with these huge category covers? It's supposed to be wide banner, not huge square covering whole screen #49 |
If anyone from the maintainer/committer team wants to commit something or work on that naming, feel free, otherwise I'll do it later |
Yeah I would also do some sensible names. ;-) |
cba3c4b
to
8e06c6b
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.
Looks good! Let's get this merged!
Every changes have been addresses