vtex-apps / product-summary

VTEX Product Summary app
11 stars 52 forks source link

Feature/fetchpriority #386

Closed LPastine closed 9 months ago

LPastine commented 9 months ago

What problem is this solving?

The goal of this change is to improve LCP performance metric on Search Pages for desktop and mobile devices.

This is achieved by setting the Product Summary Images' fetchpriority to high for the first Product Summary on mobile and to the first three on desktop, and fetchpriority to low for the rest of the Product Summary Images. As well as setting loading to eager when this condition is fulfilled.

By following the recommendation of CWV regarding the elimination of resource load delay, it is expected to improve LCP metric on Search Pages on mobile devices, as on this context the first Product Summary Image is recurrently the LCP element, and this is expected to improve Load Delay metric.

To achieve this, it's needed that the Product Summary Context dispatches the Product Summary's position to effectively assess which is the first Product Summary Image. This was already approached on PR: https://github.com/vtex-apps/product-summary-context/pull/25

How to test it?

Whirlpool Poland Workspace

Whirlpool France Workspace

Whirlpool Italy Workspace

Hotpoint Italy Workspace

Bauknecht Germany Workspace

Hotpoint UK Workspace

Screenshots or example usage:

fetchpriority-plp

Related to / Depends on

Product Summary Context PR: https://github.com/vtex-apps/product-summary-context/pull/25

vtex-io-ci-cd[bot] commented 9 months ago

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

LPastine commented 9 months ago

Could you document this and allow the fetchpriority to be set via Site Editor as well?

This PR is a good example. Thank you

Yes, I'll do it. I can't see any URL reference of the PR you're commenting but I have this PR as reference: https://github.com/vtex-apps/store-image/pull/85. I'll use this as reference and update with the changes.

LPastine commented 9 months ago

Could you document this and allow the fetchpriority to be set via Site Editor as well?

This PR is a good example. Thank you

Added the documentation and the site editor configuration:

fetch

gvc commented 9 months ago

@LPastine last hurdle before merging, can you fix the linting errors:

/github/workspace/react/ProductSummaryImage.tsx
Error:   320:10  error    Replace `·product,·position·}:·{·product:·ProductSummaryTypes.Product,·position:·number·|·undefined` with `⏎····product,⏎····position,⏎··}:·{⏎····product:·ProductSummaryTypes.Product⏎····position:·number·|·undefined⏎·`                                        prettier/prettier
Error:   434:29  error    Replace `isMobile:·boolean,·position:·number·|·undefined` with `⏎····isMobile:·boolean,⏎····position:·number·|·undefined⏎··`                                                                                                                                      prettier/prettier
Error:   434:29  error    'isMobile' is already declared in the upper scope                                                                                                                                                                                                                 no-shadow
Error:   434:48  error    'position' is already declared in the upper scope                                                                                                                                                                                                                 no-shadow
Error:   436:22  error    Replace `·?·(position·===·1·?·'high'·:·'low')·:·position·<·4·?·'high'·:·'low';` with `⏎········?·position·===·1⏎··········?·'high'⏎··········:·'low'⏎········:·position·<·4⏎········?·'high'⏎········:·'low'`                                                     prettier/prettier
Warning:   438:5   warning  Expected blank line before this statement                                                                                                                                                                                                                         padding-line-between-statements
Error:   438:[17](https://github.com/vtex-apps/product-summary/actions/runs/7210587676/job/19677642065?pr=386#step:8:18)  error    Delete `;`                                                                                                                                                                                                                                                        prettier/prettier
Error:   439:4   error    Delete `;`                                                                                                                                                                                                                                                        prettier/prettier
Error:   464:30  error    Replace `fetchpriority·===·'byPosition'·?·getFetchPriority(isMobile,·position)·:·fetchpriority` with `⏎················fetchpriority·===·'byPosition'⏎··················?·getFetchPriority(isMobile,·position)⏎··················:·fetchpriority⏎··············`  prettier/prettier
Error:   573:68  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier
Error:   576:29  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier
Error:   578:28  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier
LPastine commented 9 months ago

@LPastine last hurdle before merging, can you fix the linting errors:

/github/workspace/react/ProductSummaryImage.tsx
Error:   320:10  error    Replace `·product,·position·}:·{·product:·ProductSummaryTypes.Product,·position:·number·|·undefined` with `⏎····product,⏎····position,⏎··}:·{⏎····product:·ProductSummaryTypes.Product⏎····position:·number·|·undefined⏎·`                                        prettier/prettier
Error:   434:29  error    Replace `isMobile:·boolean,·position:·number·|·undefined` with `⏎····isMobile:·boolean,⏎····position:·number·|·undefined⏎··`                                                                                                                                      prettier/prettier
Error:   434:29  error    'isMobile' is already declared in the upper scope                                                                                                                                                                                                                 no-shadow
Error:   434:48  error    'position' is already declared in the upper scope                                                                                                                                                                                                                 no-shadow
Error:   436:22  error    Replace `·?·(position·===·1·?·'high'·:·'low')·:·position·<·4·?·'high'·:·'low';` with `⏎········?·position·===·1⏎··········?·'high'⏎··········:·'low'⏎········:·position·<·4⏎········?·'high'⏎········:·'low'`                                                     prettier/prettier
Warning:   438:5   warning  Expected blank line before this statement                                                                                                                                                                                                                         padding-line-between-statements
Error:   438:[17](https://github.com/vtex-apps/product-summary/actions/runs/7210587676/job/19677642065?pr=386#step:8:18)  error    Delete `;`                                                                                                                                                                                                                                                        prettier/prettier
Error:   439:4   error    Delete `;`                                                                                                                                                                                                                                                        prettier/prettier
Error:   464:30  error    Replace `fetchpriority·===·'byPosition'·?·getFetchPriority(isMobile,·position)·:·fetchpriority` with `⏎················fetchpriority·===·'byPosition'⏎··················?·getFetchPriority(isMobile,·position)⏎··················:·fetchpriority⏎··············`  prettier/prettier
Error:   573:68  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier
Error:   576:29  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier
Error:   578:28  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier

Yes sure, I'll fix them right away

LPastine commented 9 months ago

@LPastine last hurdle before merging, can you fix the linting errors:

/github/workspace/react/ProductSummaryImage.tsx
Error:   320:10  error    Replace `·product,·position·}:·{·product:·ProductSummaryTypes.Product,·position:·number·|·undefined` with `⏎····product,⏎····position,⏎··}:·{⏎····product:·ProductSummaryTypes.Product⏎····position:·number·|·undefined⏎·`                                        prettier/prettier
Error:   434:29  error    Replace `isMobile:·boolean,·position:·number·|·undefined` with `⏎····isMobile:·boolean,⏎····position:·number·|·undefined⏎··`                                                                                                                                      prettier/prettier
Error:   434:29  error    'isMobile' is already declared in the upper scope                                                                                                                                                                                                                 no-shadow
Error:   434:48  error    'position' is already declared in the upper scope                                                                                                                                                                                                                 no-shadow
Error:   436:22  error    Replace `·?·(position·===·1·?·'high'·:·'low')·:·position·<·4·?·'high'·:·'low';` with `⏎········?·position·===·1⏎··········?·'high'⏎··········:·'low'⏎········:·position·<·4⏎········?·'high'⏎········:·'low'`                                                     prettier/prettier
Warning:   438:5   warning  Expected blank line before this statement                                                                                                                                                                                                                         padding-line-between-statements
Error:   438:[17](https://github.com/vtex-apps/product-summary/actions/runs/7210587676/job/19677642065?pr=386#step:8:18)  error    Delete `;`                                                                                                                                                                                                                                                        prettier/prettier
Error:   439:4   error    Delete `;`                                                                                                                                                                                                                                                        prettier/prettier
Error:   464:30  error    Replace `fetchpriority·===·'byPosition'·?·getFetchPriority(isMobile,·position)·:·fetchpriority` with `⏎················fetchpriority·===·'byPosition'⏎··················?·getFetchPriority(isMobile,·position)⏎··················:·fetchpriority⏎··············`  prettier/prettier
Error:   573:68  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier
Error:   576:29  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier
Error:   578:28  error    Insert `,`                                                                                                                                                                                                                                                        prettier/prettier

@gvc lint errors fixed!

gvc commented 9 months ago

@beatrizmaselli @LPastine there are conflicts, can you resolve them?

beatrizmaselli commented 9 months ago

@beatrizmaselli @LPastine there are conflicts, can you resolve them?

solved :))

vtex-io-ci-cd[bot] commented 9 months ago

Your PR has been merged! App is being published. :rocket: Version 2.88.0 → 2.89.0

After the publishing process has been completed (check #vtex-io-releases) and doing A/B tests with the new version, you can deploy your release by running:

vtex deploy vtex.product-summary@2.89.0

After that your app will be updated on all accounts.

For more information on the deployment process check the docs. :book: