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 page - gallery zoom photoswipe #436

Merged

Conversation

JBLach
Copy link
Contributor

@JBLach JBLach commented Jan 20, 2023

Questions Answers
Description? If I understood the task correctly, I added a slider for the photo gallery in the modal, after clicking on the icon, we should see the correct photo and the modal with the slider, I also found a bug when we added the display flex to the class active in slider, it looks wrong. below is an example
Type? bug fix / improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #421

Active class with display: flex;

bug.mp4

Active class without display: flex;

correct.mp4

Appearance after changes

how-it-works.mp4

Mobile view

mobile_photo

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 21, 2023

@JBLach The idea is to implement photoswipe fullscreen gallery, with zoom capabilities etc.

Check my implementation here - https://eshop-blanco.cz/drezy-s-odkapem/17943-kuchynsky-drez-blanco-faron-xl-6-s-antracit-4020684703345.html

@JBLach
Copy link
Contributor Author

JBLach commented Jan 27, 2023

@JBLach The idea is to implement photoswipe fullscreen gallery, with zoom capabilities etc.

Check my implementation here - https://eshop-blanco.cz/drezy-s-odkapem/17943-kuchynsky-drez-blanco-faron-xl-6-s-antracit-4020684703345.html

Hi @Hlavtox,
Is the photoswipe library neccessary here? I think the only thing it adds to us is the zoom option. I don't think this is the optimal solution, unless there's something else that I don't know :)

@SharakPL
Copy link
Contributor

IMO it would be better to just use the zoom directly on the main image container without opening it on full screen backdrop on mobile. Only on desktops there should be option to open big size image besides zooming.

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 27, 2023

@SharakPL Try it on my store, you can zoom properly by gestures, swipe to change images, double tap to zoom in and out, it shows image description on the bottom etc.

@SharakPL
Copy link
Contributor

I did, but it still loads new full screen carousel which on mobile doesn't really change anything apart from disabling access to the thumbs below main image.

Screenshot_20230127-153917-373.png

IMO it should provide this zoom-in feature on doubletap directly in the main image container and full screen feature should be restricted to desktop size only.

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 27, 2023

@SharakPL How can you say that it doesnt change anything 😁

  • You can now zoom in on original image in big resolutions to see all the details. If you make only a carousel on product page, you zoom on 300x300 image.
  • You can zoom and look through the image without zooming the whole page.
  • You can swipe with touch. You will go back/forth in history on most phones.

@SharakPL
Copy link
Contributor

SharakPL commented Jan 27, 2023

Here's what I mean by zooming inside image container.

But that's just how I would like it in my theme. Your way is perfectly fine to me 🙂

kpodemski
kpodemski previously approved these changes Mar 13, 2023
Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

I think we should accept this solution. It's cleaner than using a third-party library.

WDYT @Hlavtox? Maybe we could have it configurable or something.

@Hlavtox
Copy link
Contributor

Hlavtox commented Mar 27, 2023

@kpodemski I wanted to do a real gallery plugin with zooming, swipe etc. :(

@kpodemski
Copy link
Contributor

@Hlavtox and that's cool, but it should be configurable IMHO

@NeOMakinG
Copy link
Contributor

Tbh I like it, and you could create a tutorial on the storybook to explain how you would use a third party library to achieve a more powerful feature @Hlavtox

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

I'm merging it. I think we all agree that it is an improvement, and I don't want @JBLach PR to be stale for the next half a year, especially since no one is motivated to propose a different version.

@SharakPL
Copy link
Contributor

SharakPL commented Mar 20, 2024

This would be nice to implement in the future for product gallery https://codepen.io/argyleink/pen/dyLNgpX 🤩
But better without generating shifts in the thumbnails.

Doesn't animate on Firefox yet

@dennispw
Copy link
Contributor

This would be nice to implement in the future for product gallery https://codepen.io/argyleink/pen/dyLNgpX 🤩 But better without generating shifts in the thumbnails.

Doesn't animate on Firefox yet

As long as it is completely opt-in instead of opt-out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add a "photoswipe" gallery behavior on product page images in order to be able to zoom
6 participants