-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Add Image Prefetching for Click to expand Images #61107
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
Add Image Prefetching for Click to expand Images #61107
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
68fcd2d to
5c10b84
Compare
|
@Takshil-Kunadia Thanks for creating this PR! I've left a comment on the issue to loop a few other folks into the discussion who may have thoughts on how to best proceed. In short, we previously had prefetching on hover but removed it. Since then, there's been more discussion around this, so perhaps may be good to add this back. |
5c10b84 to
e6133b0
Compare
e6133b0 to
7593d56
Compare
|
Thanks @artemiomorales ! for the context and for looping the stakeholders in the discussion! I've updated the code to reflect the direction discussed in the issue. Happy to adjust further based on any additional feedback 😄 |
27aa823 to
6dd8ed5
Compare
|
I'm just realizing a problem here with the overall image lightbox: what if the original |
3e26db9 to
3ab6bc0
Compare
|
Thanks! @westonruter for looking into this 🙇🏻♂️ and yes good catch! it makes sense to have responsive images for lightbox images to avoid unnecessarily large downloads, especially on smaller viewports. One thing I’d like to highlight though is that That said, I’ve implemented the suggested preload approach in d7f2e41 |
luisherranz
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.
Hey, just a couple of comments 🙂
westonruter
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.
Aside from the minor suggestion in #61107 (comment) this looks good to me. It is successfully preloading the image prior to opening it in the lightbox.
It can be hard to see, but in the before case the image is a bit blurry while the full size image is loaded:
| Before | After |
|---|---|
without-preload.mov |
with-preload.mov |
…ad delay constant
This reverts commit cab1562.
32fa47b to
e007c45
Compare
|
Hey @luisherranz This PR has already been reviewed and approved, so just wondering what’s next in the release process. Would love to see it land soon! |
luisherranz
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.
I'm not sure when sizes is different from 100vw for the lightbox. So, if that's not the case, it could be hardcoded.
Apart from that and a couple of nitpicks, the rest looks good to me.
|
Thanks @luisherranz implemented the changes 🙇 |
luisherranz
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.
Thanks @Takshil-Kunadia. All good on my side now 👍🙂
|
Thanks for the approval @luisherranz! 🙇 |
PR for #60883
What?
Prefetch full-resolution image to speed up display of Click to expand images.
see #60883 for more details
Why?
Hovering over a click-to-expand image block does not proactively attempt to prefetch the full-scale image in Lighbox. This can result in a prolonged period in which the low-resolution scaled-up image is displayed in the lightbox.
How?
pointerup&pointerdown.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast