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

feat: add swipe direction on iOS #1260

Merged
merged 9 commits into from
Feb 8, 2022

Conversation

WoLewicki
Copy link
Member

@WoLewicki WoLewicki commented Jan 5, 2022

Description

Added swipeDirection prop on iOS. Be aware that you need to also apply customAnimationOnSwipe: true in order for the swipe to resolve in the animation of the screen and fullScreenSwipeEnabled since you want the gesture to be recognized on the whole screen most probably. Requested in #946.

Test code and steps to reproduce

Test1260.tsx.

Checklist

TestsExample/App.js Outdated Show resolved Hide resolved
Copy link
Member

@kacperkapusciak kacperkapusciak left a comment

Choose a reason for hiding this comment

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

  1. I really like the implementation - it's very simple and elegant 👏

  2. On the other hand, the developer experience while using this prop is quite terrible. I'd suggest making all of these props set for the developer automatically by default:

fullScreenSwipeEnabled: true,
customAnimationOnSwipe: true,
stackAnimation: 'slide_from_bottom'

when setting swipeDirection to vertical.

  1. I could also see this prop as verticalSwipeEnabled but I'm okay with swipeDirection

guides/GUIDE_FOR_LIBRARY_AUTHORS.md Outdated Show resolved Hide resolved
if ([gestureRecognizer isKindOfClass:[RNSPanGestureRecognizer class]]) {
if (([gestureRecognizer isKindOfClass:[RNSPanGestureRecognizerHorizontal class]] &&
topScreen.swipeDirection == RNSScreenSwipeDirectionHorizontal) ||
([gestureRecognizer isKindOfClass:[RNSPanGestureRecognizerVertical class]] &&
Copy link
Member

Choose a reason for hiding this comment

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

As vertical swipe doesn't really make sense without fullScreenSwipeEnabled:

Screen.Recording.2022-01-26.at.09.48.44.mov

we should set it to true by default when developer sets swipeDirection to vertical

@kacperkapusciak kacperkapusciak merged commit f2ac36b into main Feb 8, 2022
@kacperkapusciak kacperkapusciak deleted the @wolewicki/ios-swipe-direction branch February 8, 2022 08:55
@hirbod
Copy link
Contributor

hirbod commented Mar 28, 2022

swipeDirection is still missing on react-navigation. Also transition duration. @kacperkapusciak would you mind to make a upstream PR?

@dylancom
Copy link
Contributor

@WoLewicki the finish transition seems to be too fast (after finishing swiping). Opposed to the slide_from_bottom animation upon opening and closing it.

@WoLewicki
Copy link
Member Author

@dylancom it might come from the fact that we use linear curve for animation so it does not ease at the end, but it is needed for the animation to correctly follow the fingers movement (see https://github.com/software-mansion/react-native-screens/pull/1260/files#diff-83118208fa3d654be0279bbef93d6715d0912aa6a5abdbf25fc5ee0eb12e1f14R213). But I think it is done the same on interactive slide_from_bottom animation so it seems weird.

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