Add prop to prevent moving child that is smaller than container #73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit does not change the behavior of `_sanitizeOffset`. It only extracts the logic to compute `minOffsetX`, `maxOffsetX` etc. into a function that can be unit tested. Since the calculation works the same for both dimensions, we make it talk about dimensions instead of width/height.
So far, when external styles force the container to be larger than the child, `_sanitizeOffset` allowed moving the child inside the container. Especially on portrait screens it can make sense to use a viewport sized container. A landscape image will initially be centered and scaled to fit into the container. Once the image is zoomed the whole viewport can be used to view the image. As long as the image still fits into the viewport vertically, it does not make sense to allow moving it vertically. There already is a prop `draggableUnZoomed` which prevents dragging the image while it has not been zoomed. Soon as the image has only slightly been zoomed, it can again be moved out of the center vertically. This commit adds a `centerContained` prop to allow ensuring that images remain centered as long as they still fit into the container. The tests for `getOffsetBounds` make sure that the new implementation behaves the same when the prop is not set.
When zooming to a factor close to one, the image is supposed to animate back to a fully unzoomed state soon as the pinch gesture ends. So far, when finishing the pinch gesture by first releasing one of the touches, the image got stuck, though. Especially when scaling the image down to a factor < 1, this looked broken since it left the image in a position that it normally can't end up in. Releasing one of the touches is handled as the beginning of a drag gesture. This is correct since it shall be possible to continue dragging the image. Once the remaining touch is released as well, `_handleDragEnd` [1] first calls `_end` to start the zoom out animation, but then it calls `_realizeInertia`. When the pinch gesture was followed by a drag gesture, velocity has been recorded and the zoom out animation is aborted [2]. To fix the problem, we reset inertia once the need for a zoom out animation has been detected. Notice that, if we are going to zoom back out, there can never be a need for an inertia animation anyway. [1] https://github.com/retyui/react-quick-pinch-zoom/blob/468eaaeb2c98bc0b26ca26b01f82b466546dbdc1/src/PinchZoom/component.tsx#L260-L261 [2] https://github.com/retyui/react-quick-pinch-zoom/blob/468eaaeb2c98bc0b26ca26b01f82b466546dbdc1/src/PinchZoom/component.tsx#L216
When using a container that is larger than the child (e.g., to build an image viewer that uses the whole viewport on portrait screens, but displays in a centered "contain" position initially), double tapping outside of the image, zoomed to an "insane" offset. To prevent this, we sanitize the offset in each step of the animation to ensure. In particular, in combination with the previous commit, the image remains in a centered positioned until it exceeds the viewport dimension and only then starts to change offset. For cases, where the container does not exceed the child size, this commit does not change anything since each intermediate offset will be "sane" anyway.
We are interested in the size of the downloaded scripts.
|
Forgot to include the video of the "after" case: screen-20230515-171648_2.mp4 |
|
Thanks! Released diff: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far, when external styles force the container to be larger than the child,
_sanitizeOffsetalways allowed moving the child inside the container:screen-20230515-171423_2.mp4
Especially on portrait screens it can make sense to use a viewport sized container. A landscape image will initially be centered and scaled to fit into the container. Once the image is zoomed the whole viewport can be used to view the image.
As long as the image still fits into the viewport vertically, it does not make sense to allow moving it vertically. There already is a prop
draggableUnZoomedwhich prevents dragging the image while it has not been zoomed. Soon as the image has only slightly been zoomed, it can again be moved out of the center vertically.This commit adds a
centerContainedprop to allow ensuring that images remain centered as long as they still fit into the container.This PR contains commits that perform the following steps:
_sanitizeOffsetlogic into a separate function unchanged and cover it with testscenterContainedprop. Tests make sure new implementation does not change behavior in previous casesFinally,
yarn sizestarted failing since the changes pushed us over the limit. So far tests were also considered in the total size, which does not make sense, I think. So I ignored those to push us under the limit. Not sure if the limit is a specifically chosen target site or if it is just about noticing when the code gets bigger.