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

feat: image pinch to zoom #1620

Merged
merged 2 commits into from
Feb 4, 2023

Conversation

JohnyTheCarrot
Copy link
Contributor

fix #1595

We should discuss the UX of this code before I mark it as ready for review, I'd reckon.

@stackblitz
Copy link

stackblitz bot commented Feb 4, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Feb 4, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit 721d53a
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/63de97fe672ca500086e62f0
😎 Deploy Preview https://deploy-preview-1620--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Feb 4, 2023

Deploy Preview for elk-docs ready!

Name Link
🔨 Latest commit 721d53a
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/63de97fefb6a600009233580
😎 Deploy Preview https://deploy-preview-1620--elk-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@danielroe danielroe 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 great - thank you! I wonder about disabling rotation - I think it might feel more stable if it was just scaling that we applied?

@Shinigami92
Copy link
Member

Yes! that's it, but pls disable rotation

@Shinigami92 Shinigami92 added c: feature Request for new feature c: ui Enhancing UI labels Feb 4, 2023
@peterbud
Copy link
Contributor

peterbud commented Feb 4, 2023

FYI: a PR with similar approach #1367 has been closed recently due to @vueuse/gesture is "not enough battle tested"

@Shinigami92
Copy link
Member

FYI: a PR with similar approach #1367 has been closed recently due to @vueuse/gesture is "not enough battle tested"

I don't know, but maybe it's just better implemented now 🤷
I tested on my iOS and even in the current state it's a BIG improvement
just the rotation should also be disabled and we should merge this ASAP

@patak-dev
Copy link
Member

@wheatjs was working on an implementation using another lib IIUC. @wheatjs, maybe you could check how this PR works and we could move forward with it if it works ok enough? And later on, we can improve on it if there is something better to put on the table.

@wheatjs
Copy link
Member

wheatjs commented Feb 4, 2023

@wheatjs was working on an implementation using another lib IIUC. @wheatjs, maybe you could check how this PR works and we could move forward with it if it works ok enough? And later on, we can improve on it if there is something better to put on the table.

I was just talking to @userquin about this and agreed that it should be merged. I think we should disable the rotation first but after that I think it is good enough for now. It will be easier to iterate on this kind of thing ones was have the basic functionality implemented. So after rotation is removed, LGTM

@JohnyTheCarrot
Copy link
Contributor Author

JohnyTheCarrot commented Feb 4, 2023

Are we certain this is good enough? There is a lack of navigating around a zoomed in image (I.e. focussing on a detail in an image in say the top left), but I am uncertain how we would tackle such a thing in a non frustrating way while somehow figuring out whether the user intended to slide to another image or wanted to move the image view around.

@userquin
Copy link
Member

userquin commented Feb 4, 2023

LGTM (if rotation removed), is there any chance to include also swap on desktop via mouse?

@JohnyTheCarrot
Copy link
Contributor Author

Rotation has been removed with my last commit, but what do you mean by swap on desktop?

@userquin
Copy link
Member

userquin commented Feb 4, 2023

how about zooming with double tap/click (back to initial)?

@userquin
Copy link
Member

userquin commented Feb 4, 2023

Rotation has been removed with my last commit, but what do you mean by swap on desktop?

try this on desktop with mouse click + drag left/right https://swiperjs.com/

@JohnyTheCarrot
Copy link
Contributor Author

Rotation has been removed with my last commit, but what do you mean by swap on desktop?

try this on desktop with mouse click https://swiperjs.com/

Do you mean to suggest to use the library behind this as well, or just that the behavior should be like that?

@wheatjs
Copy link
Member

wheatjs commented Feb 4, 2023

Are we certain this is good enough? There is a lack of navigating around a zoomed in image (I.e. focussing on a detail in an image in say the top left), but I am uncertain how we would tackle such a thing in a non frustrating way while somehow figuring out whether the user intended to slide to another image or wanted to move the image view around.

I think right now just getting a basic implementation is what is most important. The app is still in alpha and it is a good time to iterate on these types of features. Also this will hit canary before hitting the main branch, so I think it is okay

@userquin
Copy link
Member

userquin commented Feb 4, 2023

Do you mean to suggest to use the library behind this as well, or just that the behavior should be like that?

I'm asking you if we can have mouse click + drag in this PR, the link was to show you an example, this library is the alternative we're checking (@wheat and me)

EDIT: we can include zooming and mouse click + drag in next iteration

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

I just made a blackbox QA and did not check the code / implementation
IMO this is a HUGE improvement ❤️

@JohnyTheCarrot JohnyTheCarrot marked this pull request as ready for review February 4, 2023 18:00
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Lets merge and iterate. Thanks @JohnyTheCarrot for pushing this forward 🧡

@patak-dev patak-dev merged commit 2cf8f59 into elk-zone:main Feb 4, 2023
@JohnyTheCarrot
Copy link
Contributor Author

No problem, glad to help!

@JohnyTheCarrot JohnyTheCarrot deleted the feat/image-pinch-to-zoom branch February 4, 2023 18:06
@ushuz
Copy link
Contributor

ushuz commented Feb 12, 2023

Thanks for taking on the problem.

However, this change is behaving weirdly on my desktop (Edge 110 on macOS). After pinch to zoom in and out, images could get upside down, and after that, any pinch action only enlarges the image. Here's a screen recording:

screen.mp4

BTW, does anyone know this library? It's performing amazingly on my Mac, iOS Safari and Android Chrome, not sure about other devices.
https://photoswipe.com/vue-image-gallery/
https://vue-hatnqg.stackblitz.io/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature c: ui Enhancing UI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Make images in preview zoomable via pinch zoom
8 participants