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

Fix broken two touch zoom for Attachment Images #4440

Merged
merged 4 commits into from
Aug 10, 2021
Merged

Conversation

Jag96
Copy link
Contributor

@Jag96 Jag96 commented Aug 6, 2021

@marcaaron will you please review this?

Details

This PR fixes a regression where zooming with two fingers on an attachment was broken.

Fixed Issues

$ #2499

Tests/QA

  1. Sign into an account and send an image attachment
  2. After the attachment uploads, tap on it, confirm an attachment modal shows
  3. Double tap on the image, confirm it zooms
  4. Double-tap again, confirm it zooms out
  5. Use two fingers to zoom in, confirm the image zooms in
  6. While zoomed in, swipe down and right and confirm the modal doesn't close, and that the viewport pans around the zoomed image
  7. Double-tap again then swipe down or right, confirm the modal dismisses
  8. Confirm that there are no regressions on web
    1. Confirm attachments still show up on web
    2. Confirm the modal attachment closes normally on web

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

@Jag96 Jag96 requested a review from marcaaron August 6, 2021 01:23
@Jag96 Jag96 requested a review from a team as a code owner August 6, 2021 01:23
@Jag96 Jag96 self-assigned this Aug 6, 2021
@MelvinBot MelvinBot requested review from yuwenmemon and removed request for a team August 6, 2021 01:23
yuwenmemon
yuwenmemon previously approved these changes Aug 6, 2021
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Looks good there's just a small style guide rule we should fix up

src/components/ImageView/index.native.js Outdated Show resolved Hide resolved
src/components/ImageView/index.native.js Outdated Show resolved Hide resolved
Create an invisible view on top of the image so we can capture and set the amount of touches before
the ImageZoom's PanResponder does. Children will be triggered first, so this needs to be inside the
ImageZoom to work
*/}
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa, nice workaround

@Jag96
Copy link
Contributor Author

Jag96 commented Aug 9, 2021

Updated!

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM 🤘

@Jag96 Jag96 merged commit 347a208 into main Aug 10, 2021
@Jag96 Jag96 deleted the joe-two-finger-zoom branch August 10, 2021 01:34
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Jag96 in version: 1.0.83-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

4 participants