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

Pinch to zoom #1952

Merged
merged 7 commits into from
Aug 22, 2024
Merged

Pinch to zoom #1952

merged 7 commits into from
Aug 22, 2024

Conversation

starypatyk
Copy link
Contributor

@starypatyk starypatyk commented Aug 28, 2023

Here is my initial pinch-to-zoom implementation. This should resolve #916 and #917.

I have not committed the compiled code yet, to make rebasing easier.

Some explanation of the changes:

In 003f31f I have switched from mouse events to more generic pointer events (see https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events).

e96662c is the actual implementation.

  • I have re-used the original zoom handling code - a significant part of the old updateZoom method was extracted to updateZoomAndShift and I created an additional common method updateShift.
  • Co-ordinates of the two touches are kept in pointerCache.
  • Pinch-zoom calculations are explained in the comments.

I committed aeab2bd separately, to make it easier to follow the changes.

1f0fced was required after recent changes in master - as I removed the width/height: 100% from CSS.

I have the following questions:

  • Regarding 1f0fced - an alternative solution would be to bring back width/height: 100%. I am not sure which one is better. Any suggestions?
  • The pointerMove and pointerUp handlers are now attached all the time. I am not sure what should be done here:
    window.addEventListener('pointerout', this.pointerUp)
    and here:
    window.removeEventListener('pointerout', this.pointerUp)
    Probably also a permanent handler for pointerOut that would invoke resetZoom? I do not know how to test this case. Suggestions welcome.
  • I have tested on Android and iPad. Would be good to verify on an iPhone.
  • I have noticed that recent changes to NcModal (fix(NcModal): Close button should be visible even if modal content is scrolled nextcloud-libraries/nextcloud-vue#4350) affect zooming behaviour. Now, when the image is enlarged a bit from 100%, a scrollbar is shown, and this causes a slight shift in image position. This happens on Chrome and Chrome Mobile (for some reason Firefox does not show the scrollbar). This is not a major problem, but is quite a bit annoying. I wonder if we should try to have some workaround in this PR or leave it for a separate PR.

@starypatyk starypatyk added enhancement New feature or request 2. developing Work in progress labels Aug 28, 2023
@starypatyk starypatyk requested a review from skjnldsv August 28, 2023 21:08
@github-actions
Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the reviewing process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR reviewing process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@szaimen szaimen added this to the Nextcloud 28 milestone Sep 13, 2023
@skjnldsv skjnldsv removed this from the Nextcloud 28 milestone Sep 19, 2023
@skjnldsv
Copy link
Member

@starypatyk I don't have much time for Viewer lately, thank you for this PR (and your patience 🙈)

@starypatyk
Copy link
Contributor Author

@skjnldsv No worries. 😉 Thanks for letting me know. Waiting patiently then. 🐌

@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@starypatyk
Copy link
Contributor Author

starypatyk commented Mar 7, 2024

@starypatyk I don't have much time for Viewer lately, thank you for this PR (and your patience 🙈)

Hi @skjnldsv,

Any chance to go forward with this PR?

FYI - I have rebased and re-checked the implementation. Seems to behave decently. Tested as previously on Android and iPad.

This still requires some clean-up - see my questions in the first comment (I have updated commit references after rebase in the original comment).

@skjnldsv skjnldsv removed the 2. developing Work in progress label Aug 22, 2024
@skjnldsv
Copy link
Member

/compile /

@skjnldsv
Copy link
Member

Patience came to an end, let get this in!
Sorry again @starypatyk!

Btw, if you're interested, I sketched an RFC for an improved Viewer version 3.0.
If you have some commens or feedback, feel free to check #2395 🎉

@skjnldsv
Copy link
Member

cypress fix: #2436

@skjnldsv skjnldsv added the 4. to release Ready to be released and/or waiting for tests to finish label Aug 22, 2024
@skjnldsv skjnldsv enabled auto-merge August 22, 2024 12:40
@skjnldsv
Copy link
Member

Fixing the public view rendering :)

@skjnldsv skjnldsv disabled auto-merge August 22, 2024 14:21
@skjnldsv skjnldsv merged commit 5655193 into master Aug 22, 2024
11 checks passed
@skjnldsv skjnldsv deleted the enh/pinch-to-zoom branch August 22, 2024 14:21
@starypatyk
Copy link
Contributor Author

@skjnldsv - Great to see this merged finally! 😉 Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pinch to zoom and pan image previews (mobile)
5 participants