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

Changed size with dynamic values and lazy loading with fetchpriority #135

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

buggyzap
Copy link
Contributor

Questions Answers
Description? Width and height can't be static but depends on image resize so I have changed it with dynamic values coming from controller. I have also changed the lazy loading with the attribute fetchpriority=high because, with new SEO standard we except that the cover image, ideally is in the visible First Contentful Paint and we need to load category cover image as soon as possible, to distinguish other images from cover we need to tell what to load first thanks with this attribute.
Type? improvement
BC breaks? no
Deprecations? no
How to test? At page load we can see new values, testing the page with Google audit we notice that in network the cover image is loaded before other images.

@ps-jarvis ps-jarvis added the Waiting for QA Status: Waiting for QA feedback label Jan 23, 2024
@paulnoelcholot
Copy link

Hello @buggyzap,

I want to test your PR but I don't understand. Can you provide me with clear steps to reproduce allowing me to see the changes?

Thanks for your feedback! 🙏

@paulnoelcholot paulnoelcholot added Waiting for author and removed Waiting for QA Status: Waiting for QA feedback labels Jan 30, 2024
@buggyzap
Copy link
Contributor Author

Hello @buggyzap,

I want to test your PR but I don't understand. Can you provide me with clear steps to reproduce allowing me to see the changes?

Thanks for your feedback! 🙏

Hi @paulnoelcholot,

there are 2 improvements on this PR, a change in image size that was hardcoded in 141x180 ( you can look at file changes ) and the attribute loading=lazy that is changed in fetchpriority=high.

To reproduce the first bug

  1. Go to backoffice -> Design -> Image settings
  2. Select category_default resize and edit
  3. Change dimensions with a different values, eg. 300x300
  4. Save and regenerate category_default miniatures
  5. Open any category in backoffice and upload a cover image
  6. If you inspect the code, the size is not updated because are hardcoded. With my PR you got updated values 300x300 in yout width/height.

To see changes on second improvement, you need to know what loading and fetchpriority attributes mean.

I will try to resume to you, the previous attribute "loading=lazy" tells to browser to load the image only if it is visible to viewport to the end-user, instead, the "fetchpriority=high" is useful to tells to browser that this image, is a most important image of the page, so it is critical to load it first because its view will impact the LCP time.

LCP is the Largest Contentful Paint, the element in the viewport viewed by end-user will impact this metrics in Web Vitals.

To accomplish this task we can set it to fetchpriority=high and write a most modern and better code with these new attributes.

Thanks

@buggyzap buggyzap requested a review from Hlavtox March 14, 2024 14:42
@buggyzap
Copy link
Contributor Author

Hi @paulnoelcholot friendly reminder

@buggyzap
Copy link
Contributor Author

Hi @paulnoelcholot friendly reminder...

@Hlavtox
Copy link
Contributor

Hlavtox commented Apr 30, 2024

@Hlavtox Hlavtox merged commit 8e98738 into PrestaShop:develop Apr 30, 2024
6 checks passed
@Hlavtox Hlavtox added this to the 3.0.0 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants