Add ability to zoom images on web/desktop #2085
Conversation
marcaaron
changed the title
|
Tagging in @roryabraham too since he's got some experience with UI widget guys in E.cash 😄 |
|
@marcaaron I was testing this on mobile web on my iPhone and noticed it had some unintended consequences. It's a bit hard to describe, so I suggest you try it by creating an ngrok tunnel to your localhost:8080 and visiting that tunnel on your phone. On iPhone on mobile web and iOS right now, you can zoom into the images normally, without the changes in this PR. But with this PR, if you do that normal "pinch zoom", it recognizes it as a mouse-click, zooms the image (I think where you first touched the screen), and has a weird jumping effect. I think basically we want to make sure we're not doing anything on tap, while on mouse-click we want to zoom in. |
src/components/ImageView/index.js
Outdated
| ImageView.displayName = 'ImageView'; | ||
| componentDidMount() { | ||
| document.addEventListener('mousemove', this.trackMouseMove.bind(this)); | ||
| document.addEventListener('touchmove', this.trackMouseMove.bind(this)); |
There was a problem hiding this comment.
Why did you need to set up a listener for touchmove?
There was a problem hiding this comment.
So that mobile web touches would set isDragging
There was a problem hiding this comment.
But maybe there is a solution where we don't need to do this and we can tell what kind of mouse event we've got.
|
Thanks for testing that. I guess my thinking here was that we sort of unofficially support pinching and zooming now, but that's probably more useful on mWeb than what these changes are providing to desktop/web users. So, maybe we should just disable this for mobile web and let people pinch/zoom normally for now? Not sure if there is an easy way to identify web vs. mobile web besides "screen width". |
|
Nice! |
|
@roryabraham came up with a temporary solution for the mobile web issue by adding a utility that detects if we have a touch screen. If we do, we'll assume that we can use that to zoom in on images and not utilize the feature, if not, then we'll allow the mouse zoom and drag feature instead. Let me know what you think. |
|
I will also caveat this by saying I think the long term solution here will be to use a single component for native/mWeb eventually. But didn't want to dive down that rabbit hole at the expense of quickly improving the desktop/web experience. |
| super(props); | ||
| this.scrollableRef = null; | ||
| this.canUseTouchScreen = canUseTouchScreen(); | ||
| this.containerStyles = [ |
There was a problem hiding this comment.
Only NAB is it seems like this should be put in a style file instead of declared in the constructor.
Ah no, I'm suggesting that native and mobile web use the same exact component to handle touches + pinch zoom with something like this which has web support, but still has a few kinks to work out. |
|
Oh, nice. Yeah that makes more sense to me. Anyways, this LGTM and tested well 👍 |
Details
Adds ability to zoom to 200% by clicking on image attachment on web/mWeb
Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/146569
Tests
web/desktop
mWeb
Tested On
Screenshots
Web
2021-03-26_10-08-18.mp4
Mobile Web
Desktop
2021-03-26_10-08-18.mp4
iOS
Android