-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 zoom on hover and show spinner when image loading #2314
Conversation
5f030b2
to
a953538
Compare
a953538
to
0bbd5df
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.
LGTM with a couple small comments addressed
1a3c1d2
to
f183488
Compare
f183488
to
060666e
Compare
Aligned with Monica this morning on:
To keep an eye on This feature is unavailable on mobile, so only desktop checked. |
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.
Commented above.
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.
Looking great! Tested and it's working well for both transparent and non-transparent images, and also with different background colors.
I left just one main comment about a few extra scenarios we need to cover. A simple change to our loading spinner id
structure should fix them all, I believe.
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.
Works exactly as expected now in all the different product page configurations I've tested. Thanks for making the changes! :D
* shopify/main: update version and release notes (Shopify#2327) add zoom on hover and show spinner when image loading (Shopify#2314) Try more universal rich text controls (Shopify#2085) Hosted video support in video section (Shopify#2248)
* add zoom on hover and show spinner when image loading
PR Summary:
Fixes an issue with transparent product images when the "Click and hover" image zoom option is activated.
Why are these changes introduced?
Fixes #2301
Just like the original image, the magnified overlay currently uses a transparent background. This causes the thumbnail in the background to become visible, which looks buggy. To fix the issue, I applied a solid background to all images, matching the background colour chosen in Theme settings.
The PR also adds a loading spinner to each thumbnail. When a larger image is loading, the thumbnail opacity changes to 50% and the loading spinner is displayed. Once the image is loaded, this disappears and the thumbnail opacity goes back to 100%.
What approach did you take?
backgroundColor
tovar(--gradient-background)
Decision log
data-media-id
to the spinner and use that to query.parentElement.parentElement
is not very elegant. It's not immediately obvious why we're doing it this wayVisual impact on existing themes
Merchants who upgrade to a new theme version with this change will get a new "Click and hover" option under their "Image zoom" settings.
Testing steps/scenarios
Art Deco - Cyclamen
,Bo Ivy - Brown
, orPleated Heel Boot
) and navigate to its product pageTransparent background PNG
product page and click the thumbnail to zoom in. Verify that the thumbnail is not showing behind the zoomed-in image, and that the background matches the colour chosen in Theme settings.Demo links
Transparent image
transparent.mp4
Regular image with spinner
regular.image.mp4
Checklist