Better Unzoom on drag controls.

[tshmieldev]

Description

Hello.
The changes I made are caused by bad user experience on mobile, when the image zooms, and the user tries to pinch the screen, to have a better look at the contents. In its current state, this is recognized as dragging the image, and an event handler makes sure to unzoom the message, when the distance dragged by the user is > threshold.
The changes I propose, are:

• Adding that threshold to props (unZoomOnContentDraggedThreshold, default = 10 (old, hardcoded value))
• Adding an option to disable that behavior to props (unZoomOnContentDragged, default = true (old value behavior))

These changes make it, so with proper configuration, we can provide amazing user experience on mobile, users can zoom in with pinching as much as they want (or as much as you will let them with the unZoomOnContentDraggedThreshold), and still close the image by any other means (clicking on it, or the minimize button).

One change that I am not sold on is unZoomOnContentDraggedThreshold. Yes, it gives nice granular control, but I suppose that many people would want to disable the drag behavior, or keep it as it is. Feel free to keep that as it was, as it is not my priority with this pull request.

I hope that you find my changes meaningful and useful.

Testing

Tested with the stories app, works as expected

[rpearce]

Hi! I will play with this in a few days when I get time. If this does what you say, then it might be able to fix this issue: #436. Thank you for your contribution, and I'll take a look when I can.

[rpearce]

In the mean-time, it looks like the build is failing with these new editions, so you might want to try resolving that: https://github.com/rpearce/react-medium-image-zoom/actions/runs/8351174329/job/22915694471?pr=472#step:5:57

[tshmieldev]

@rpearce got it, I will take a better look at why that is

[tshmieldev]

@rpearce corrected the lint errors, Lint passes beautifully. I tested it on the storybook project and it works (can't npm build because of windows...), I would suggest perhaps making the unZoomOnContentDragged true by default, as I don't see why someone would want the old behavior

[rpearce]

Thanks for the quick work. I may be able to look at this closer in ~10 hours

[tshmieldev]

@rpearce any updates?

[rpearce]

I haven't had time. I'm aiming to review tonight.

[rpearce]

@tshmieldev I've opened a competing or complimentary PR (I'm not sure which, yet) that might allow us to have this behavior without requiring a property. If you're able to test it, that would be much appreciated.

If that PR actually works to solve the problem, do you think it still makes sense to have these properties? I have some thoughts, but I'd like to hear yours first!

[tshmieldev]

@rpearce I think the new behavior without properties is perfect, because who would need to change the behavior. I will test in ~8 hours

[tshmieldev]

@rpearce perhaps having the unzoom threshold as a prop would he useful

[rpearce]

That's what I think, too. I need to test that PR more, but I won't have time to look at this project again for ~13 hours or 24 past that.

In the mean time, if you like what you see there, maybe we could alter this one to focus on the threshold only?

[tshmieldev]

@rpearce I propose making the behavior as such:
One new prop, instead of the two I proposed: "unzoomOnDragThreshold", defaults to null. If it is set, the old dragging behavior takes place, taking into account the prop. If it is null, no umzoom on drag.

[rpearce]

@tshmieldev Thank you for your thoughts. I would prefer to be explicit over implicit. After thinking about this, I can think of a few reasons to disable the unzoom on drag (what this PR does, plus specifying a threshold), and I see the other PR as the default behavior, for it's been a known bug for ages.

So, here's what I'd like to do, if you are willing:

1. I'll merge the other PR and release a patch-level release
2. Keep this PR (this is where you come in)
   1. Rename unZoomOnContentDragged to canSwipeToUnzoom and default it to true
   2. Rename unZoomOnContentDraggedThreshold to swipeToUnzoomThreshold and default it to 10
   3. Merge in the change from the main branch (I don't know your skill level, so let me know if you need help here) and fix the merge conflict that will likely be in the handleTouchMove handler
3. I'll merge this PR once it's ready
4. I'll make a PR that includes example stories covering these properties
5. Once 👆 is merged, I'll make a minor-level release that includes you as a contributor in package.json and in the "All Contibutors" section in the README

Please let me know if you have issue with any of this.

[tshmieldev]

@rpearce I am happy to help, but I am pretty new to git, and I'm not sure if I'll be able to handle merging main, but I can surely rename the variables as you specified

[rpearce]

Ok, let's try. Have you read through this? https://github.com/rpearce/react-medium-image-zoom/blob/main/CONTRIBUTING.md

In your fork, you want to set the upstream branch, fetch it, then rebase your main branch to this one's main branch (all of that is in the doc). Then, in your feature branch, you git merge main, fix the conflict(s), create a new commit, and push that to your feature branch.

I'm currently feeding a baby a bottle, so if this isn't clear, it might be better to close this PR and open a new one that references it.

I may also be able to edit this PR, if you get stuck and want me to try.

[tshmieldev]

@rpearce thank you for your help, I made my local repo according to the contributing guide, I will try to merge when I get back home, ~2 hours

[tshmieldev]

@rpearce Ready to be merged into main, tested with the storybook, seems to be working just fine

[rpearce]

Actually, don't worry about that. I can change that, myself, once this is merged.

This git diff is very strange, though...

[rpearce]

I released react-medium-image-zoom@5.1.11 tonight with the other fix, but I'm out of time for this. I won't be able to do anything tomorrow night with this, but I may be able to work it into my day. If not, it'll have to wait until the following night.

I apologize for the time delay, but work & real life things take precedence.

[rpearce]

If you're willing, to avoid you having to do git things, I think it would be wise to open a new branch and a new PR with only the changes we want to make showing up, including that little change I mentioned above.

If you don't have time to do that or can't, I can do it and be sure to cite you and this PR.

Let me know what you want to do; ideally, this PR diff should only show the intended changes coming in.

[tshmieldev]

@rpearce Added the change, seems like in the merge, the if statements were duplicated. Hope that the branch is good now, if not I will make a new branch and PR from there

[rpearce]

This looks pretty good now, and I'll make a couple of aesthetic changes when I merge it. I'll try to get to it in about 45 hours when I free up again, but the other fix should already be out there and ready for use.

[rpearce]

@tshmieldev Thank you for your participation here! Some notes:

• I adjusted a few things and added storybook stories here: chore: backfill docs and style for 2 new swipe to unzoom features #510
• I added you as an "all contributor" and NPM contributor here: chore: v5.2.0 #511

Here you are in the README!

Thanks again,
Robert