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: media preview modal - better zoom support #2133

Merged

Conversation

horvbalint
Copy link
Contributor

In this PR I tried to solve the various zoom issues in the image modal component. It was not as straightforward as I tought so there might be some bugs, but it worked very reliably for me. Feedback is welcomed 😊

Main changes are:

Pan support while zoomed in:

It is now possible to move the zoomed-in image. While zoomed-in the swipe gestures are disabled and one can only pan to the edge of the image (+ some gap)

pan-while-zoomed.mp4

Double tap zoom in/out:

This new gesture is now recognized and will either zoom the image in (2x) if it was not zoomed-in before or it will zoom out if it was zoomed-in. One can also reset the zoom state with this, if was previously zoomed-in manually.

double-tap-zoom.mp4

Desktop support

All the gestrues now also work on a desktop with a mouse or a touchpad. The previous arrow-key navigation still works.

desktop-support.webm

Other changes:

  • I removed the option to zoom the image out (to less then 1x) It can be re-added, I just found no use-case for it, so if you have please leave a reply 😊
  • The 'spring' animation is removed on zoom. This is mainly a technical limitation, but I also find it nicer this way. If it is important I can play with it some more 😅
  • I removed the threshold prop from the carousel component since it was not in use and the library defaults feel very good IMO.

Most of the code changed, I tried to clean up the new code, but it might need some more cleanup.

@stackblitz
Copy link

stackblitz bot commented May 29, 2023

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

@netlify
Copy link

netlify bot commented May 29, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit bf129bc
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/648746a58dce300008bf4a6b

@netlify
Copy link

netlify bot commented May 29, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit bf129bc
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/648746a51821f600086684fc
😎 Deploy Preview https://deploy-preview-2133--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.

@socket-security
Copy link

socket-security bot commented May 29, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@patak-dev
Copy link
Member

Thanks for the PR! I saw some glitches in desktop (chrome on M1) and Android.
Desktop for 4 tall images:

  • Zooming out will show all the images
  • You get inversion of the image at one point
  • with the mouse you can slide the image left/right until the next one, but then it jump back to the first one

Android seems to work well for tall images. For a landscape image:

  • Zoom in keeps the height of the image the same, so you only see a small portion of it
  • when loading, the placeholder doesn't respect the height so there is a layout shift (probably unrelated to this PR, just mentioning it because I saw it)

@horvbalint
Copy link
Contributor Author

horvbalint commented May 29, 2023

Thank you @patak-dev !

I could not reproduce any of this yet except for "with the mouse you can slide the image left/right until the next one, but then it jump back to the first one". I knew about this one, it happens because it is a slow movement and not a 'swipe', I will figure it out.

Can you please post some images on the Elk Dev profile, so I can test on the same images as you did?

@patak-dev
Copy link
Member

@horvbalint
Copy link
Contributor Author

@patak-dev thanks again!

I think I solved these:

  • with the mouse you can slide the image left/right until the next one, but then it jump back to the first one
  • Zoom in keeps the height of the image the same, so you only see a small portion of it

The zoom-out bug is really wierd, since I disabled zoom-out completely. The inverse thing makes sense if you zoom out too much, but you shouldn't be able to zoom-out in the first place 😅
Sadly I can not reproduce it, I've tried it on Chrome and Firefox on desktop (Linux) and Chrome on Android. Can you please give me steps on how to zoom out?

Other small changes in the last commit:

  • I removed the ability to drag the slide horizontaly if there is only one slide
  • Fixed the missing gap between the slides

@patak-dev
Copy link
Member

Works a lot better! The things I saw before are mostly gone. And I can no longer reproduce zooming out to invert the image.
What I still experience is that when I try to swipe on mobile, if I don't do it perfectly horizontally, it will close the zoomed image instead. I tried to reproduce on elk.zone, and it works fine there.

@horvbalint
Copy link
Contributor Author

@patak-dev I fixed the issue with the (close to) diagonal swipes. Now it will only close the modal if you move more on the vertical axis then on the horizontal one.
Let me now if you find more bugs, it is very useful because I am always certain that there is no more 😄

@bitbonk
Copy link

bitbonk commented May 31, 2023

I tried the preview using
https://deploy-preview-2133--elk-zone.netlify.app/home and the image zooming and panning still feels slightly awkward compared to native (iOS) apps.

There it usually behaves like this:

  1. Opening an images gives the whole screen real estate to the image, no borders, no chrome no close buttons etc. - especially the borders in Elk are suboptimal on my small iPhone 7 screen
  2. Swipe down or up to close the image view - in the Elk preview it only seems to work on swipe up
  3. double tap to zoom in zooms in way more than the Elk preview does
  4. double tap zooms in on the area I tapped on (zoom in on the lower left corner of I tapped the lower left corner) - in the Elk preview it always zooms in on the center
  5. pinch to zoom scales the image at the same rate as I pinch the fingers, so it feels like the fingers stay at the same position on the image - in the Elk preview the image scales at a different rate than my fingers
  6. panning with one finger works more like scrolling - if I lift the finger it continues to pan for a while - in the Elk preview it stops immediately
  7. I can pan and zoom at the same time with two fingers - in the Elk preview I can only zoom with pinching

Not sure all of this can be emulated using web technologies.

@horvbalint
Copy link
Contributor Author

Thanks for the feedback @bitbonk!

  1. On mobile I would also prefer a fullscreen view of the image like you described, but I am no designer so I did not want to change the design by myself, "just" fix the behaviour.
  2. Basically the same here. If that is the desired way, it would be very easy to implement.
  3. My initial tought was to make it so that if you double tap, then the image fills the view. But if the image aspect ratio was the same as the view it would not do anything, so I opted for the 2x zoom that is now. This can be any number of course (or behaviour).
  4. Good catch, I will change the implementation
  5. Hmm, for me it is scaling with the rate of my pinching for the first pinch and scales slower after that. I will try to look into this. Are you experiencing the same thing on iOS?
  6. Yeah, I noticed this just today 😄 I will try to improve it
  7. I think this is also doable, but might be difficult

It would help a lot if someone from the Elk team could provide some answers for the UI/UX questions like the zoom amount on double tap or how the modal should look like on mobile. @patak-dev @danielroe can you help with this?

Maybe if big changes are needed, we could do it as a "next step" and ship this in the meantime (after the smaller fixes) ? I think many people would like to be able to pan the zoomed image which is (almost) not possible currently. This is expressed in the issue #740

@bitbonk
Copy link

bitbonk commented Jun 3, 2023

@horvbalint

  1. Hmm, for me it is scaling with the rate of my pinching for the first pinch and scales slower after that. I will try to look into this. Are you experiencing the same thing on iOS?

Yes, that's the same behaviour on my iOS 15, iPhone 7 when I zoom/pinch very slowly.
But when I pinch quickly, it even zooms too slow for the first time. May this "lag" is related so some performance issue?

- pinch now zooms with the same rate as the fingers move
- one can now drag the image the same time as pinching
- double-tap zoom increased to 3x from 2x
- it is now possible to close with a downwards swipe
@horvbalint
Copy link
Contributor Author

@bitbonk thank you!

I had some time today and looked into it more and the correct pinch zooms were only coincidences 😄
Main changes today:

  • Pinch to zoom now zooms the same rate as your fingers move
  • It is possible to drag the image while pinching by moving your fingers

Other changes (since I got no "official" response, I implemented your ideas):

  • Double tap zoom increased from 2x to 3x
  • It is now possible to close the modal with a downward swipe

I think this is now very close to a native app 🤯

The only thing that would be much harder is your 6th point: "panning with one finger works more like scrolling - if I lift the finger it continues to pan for a while - in the Elk preview it stops immediately".
This would require us to use requestAnimationFrame and animate the image movement and scale for ourself instead of relying on the browsers transition capabilities. I am not sure if it is worth it, especially now that pinch to zoom works much better. Can you please give your opinion on this?

I don't think that an iPhone 7 should have any performance issues, so if you have some please report, that is most likely a bug.

@bitbonk
Copy link

bitbonk commented Jun 6, 2023

I just tested the latest build (https://deploy-preview-2133--elk-zone.netlify.app/) on my iPhone and to me everything looks really good now.

Good job! 👏👏

@userquin
Copy link
Member

userquin commented Jun 12, 2023

looks good, on dektop we need to drag a lot the images, maybe we can reduce it a bit: https://deploy-preview-2133--elk-zone.netlify.app/mastodon.social/@pixelfed/110450604878462337

testing android...

looks good also in android: https://streamable.com/kz2rc5

wrong video, sorry: https://streamable.com/8e0iyk

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.

Let's merge and we can let other tests more in https://main.elk.zone. Thanks for improving this feature @horvbalint! 👏🏼

@patak-dev patak-dev merged commit a94fe1c into elk-zone:main Jun 12, 2023
12 checks passed
@lizyChy0329
Copy link

But, Why didn't use Swiper for this function? Need to keep 0 dependencies?

@horvbalint
Copy link
Contributor Author

horvbalint commented Jul 24, 2023

@lizy-CZ I am not sure about the dependency policy of the project, I built upon the packages that were used in the previous version

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.

None yet

5 participants