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

[Product] Fix transparent image zoom #2280

Closed
wants to merge 5 commits into from

Conversation

LucasLacerdaUX
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX commented Feb 2, 2023

PR Summary:

Why are these changes introduced?

Transparent images are no longer duplicated when zooming in with the cursor.
Fixes #2278.

What approach did you take?

Added opacity 0 to the product image.

Testing steps/scenarios

Demo links

@kmeleta kmeleta self-requested a review February 2, 2023 21:20
@LucasLacerdaUX LucasLacerdaUX changed the title Fix image zoom [Product] Fix transparent image zoom Feb 2, 2023
@ludoboludo ludoboludo self-requested a review February 2, 2023 21:21
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I had in mind so I'm happy with the approach. Just noticed a small thing.

@@ -1158,6 +1158,10 @@ a.product__text {
border-radius: calc(var(--media-radius) - var(--media-border-width));
}

.image-magnify-full-size + img {
opacity: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a transition in scope for opacity here. https://screenshot.click/02-22-zhx81-93cmp.mp4 We could override that or probably use visibility instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah coming from here, I was noticing the same:

Screenshot

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fixed it by adding transition: none to .media>.image-magnify-hover.

  • I opted for this instead of visibility:hidden because visibility removes it from the accessibility tree.
  • Applied transition:none to .media>.image-magnify-hover instead of .image-magnify-full-size + img because in the latter, only the in transition is impacted. Once the class is removed, the out still shows up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@melissaperreault melissaperreault self-requested a review February 2, 2023 21:27
@@ -1158,6 +1158,11 @@ a.product__text {
border-radius: calc(var(--media-radius) - var(--media-border-width));
}

.image-magnify-full-size + img {
opacity: 0;
transition: none;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the images that dont have the transparent background, theres a bit of a flash now 🤔
thoughts?

https://screenshot.click/02-31-l7odb-sxi3n.mp4

Copy link
Contributor

@kmeleta kmeleta Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof I think that flash is a sign we were counting on the image as a placeholder while the full size loads https://screenshot.click/02-40-rfvqq-80apq.mp4

Here's a comparison for original behavior where we didn't hide the image https://screenshot.click/02-56-j7j37-1lz8h.mp4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more apparent if you throttle the network

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did see the flash but I don't see it anymore 🤔

Copy link
Contributor

@kmeleta kmeleta Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the full size image loads, it's cached so there's no delay to reveal the whitespace behind. Meaning slower connections would see a longer flash. Lucas' other changes have eliminated the transition issue well though.

@stufreen
Copy link
Contributor

stufreen commented Feb 2, 2023

Similar to other comments, but if the full-size image (zoomed) is not cached there is now a blank screen while it's loading:

Screen.Recording.2023-02-02.at.4.39.31.PM.mov

Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Lucas!
Works as expected except I notice something unrelated:

  • Product cards adds a background to PNG images. (Video reference) I'll create a new issue for this!

Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticing some weirdness with the load listener. It's a little inconsistent, but consistent enough to be not ideal. Recording with throttled network https://screenshot.click/02-07-tdb7m-1ynl6.mp4

@stufreen
Copy link
Contributor

@lucas let's close this one - I opened a new issue for it that @metamoni is going to pick up https://github.com/orgs/Shopify/projects/4690/views/47?pane=issue&itemId=19635258

@stufreen stufreen closed this Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image looks duplicated when Click and hover on image zoom is selected
6 participants