-
Notifications
You must be signed in to change notification settings - Fork 151
Feature/preload image pdp #1123
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
Conversation
|
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.
|
|
Beep boop 🤖 I noticed you didn't make any changes at the
In order to keep track, I'll create an issue if you decide now is not a good time
|
|
|
vmourac-vtex
left a comment
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 PR seems to be based on the same branch as of #1120.
If this was not done for a specific reason, I would ask to make them independent of each other in case we need to rollback any specific deploy
|
I would also request that you:
|
@gabridevs I kindly ask that you carefully review your work, regardless of whether it was assisted by AI or implemented manually. I also ask that you go over all the requested review changes from past interactions, and only then request the PR review again. If you have any questions, feel free to ask. |
|
Hi @vmourac-vtex, |
|
Hi @vmourac-vtex, |
Hi @gabridevs. I propose a different solution. Since the three original PRs need similar fixes (#1120, #1122, #1123) I will list a proposal for fixes on all of them here. Instead of trying to fix the current branches, lets start from scratch. We need an up-to-date You can create a fork from scratch, but GitHub won't allow you to have multiple forks of the same repo. So, if you don't want to delete your current fork and recreate it, I suggest you
Now you have a clean and up to date
Now you can copy/paste the required code from each original branch to the new feature branches. Since the original branches for preload-image and pdp-fetch-priority had code that was mixed with the custom thumbs solution, remember to only copy/paste the code that is related to the change you are proposing in that PR Open PRs for each of the new branches and we iterate from that, with a clean slate |
| <Helmet> | ||
| {index === 0 && ( | ||
| <link rel="preload" as="image" href={imageUrl(src, DEFAULT_SIZE, MAX_SIZE, aspectRatio)} /> | ||
| )} | ||
| </Helmet> |
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.
Theoretically the change makes sense, but we could not validate it in the provided workspace
Ensure that in your provided workspace we can verify that:
- We can see the preloaded image in the
headcomponent - We can ensure that the preloaded asset is clearly prioritized in the developer tools
|
hi @vmourac-vtex, I close this PR as we have developed a custom component in our store. |
What problem is this solving?
This change improves the loading performance of product images by preloading the main product image via a tag in the ProductImage component. It ensures that the primary image is fetched earlier in the page load process, reducing time-to-visual-complete and enhancing user experience, particularly on slower connections.
How to test it?
You can test this by checking the network activity in the browser’s developer tools. The main product image should be listed as a preloaded resource, and the page should render the image faster compared to the previous behavior.
Workspace
Screenshots or example usage: