-
Notifications
You must be signed in to change notification settings - Fork 444
feat: back gesture with full screen swipe #665
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: back gesture with full screen swipe #665
Conversation
Using this patch for many many months already, can confirm that it just works (TM) |
|
||
- (BOOL)gestureRecognizer:(UIGestureRecognizer *)gestureRecognizer shouldRecognizeSimultaneouslyWithGestureRecognizer:(UIGestureRecognizer *)otherGestureRecognizer { | ||
|
||
// The below snippet disables the pager view's scrollview's scroll when current index is 0 and user is swiping back. Useful for fullScreenGestureEnabled in react-native-screens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should also happen if it's the last page and the user is swiping in the other direction. It's not related to native stack but it would make it work when it's nested in something else with horizontal gestures. For example, maybe drawer on the right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay. I misunderstood what you meant. I think in that case, it'll default to self.scrollView.panGestureRecognizer.enabled = self.scrollEnabled
i.e. default scroll enabled setting (since it's not a back gesture). So I guess it should be fine. I think something like a Drawer would need to prevent the pager gesture too. Happy to test it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some quick testing with the drawer, this method doesn't run for the drawer as the gesture never reaches the page view (because of Overlay).
Screen.Recording.2022-12-04.at.6.55.04.AM.mov
Screen.Recording.2022-12-04.at.7.23.23.AM.mov
I still think that even if it runs for some horizontal gesture, it should be "mostly" fine.
Scenario - Someone adds a horizontal gesture recognizer to a parent view of the pager view.
- By default, the parent recognizer won't receive events. However, they would if a user is swiping "back" and they're on page 0 (because we'd disable the scroll). tbh I am not sure if it's a bug or a feature 😅
Edit: In the above scenario, they'd receive the simultaneous event as you mentioned in the first message, feels less safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relatively Safer alternative that works. One downside is that it depends upon the member class name for pan gesture used in RN Screens
// The below snippet disables the pager view's scrollview's scroll when current index is 0 and user is swiping back. Useful for fullScreenGestureEnabled in RN Screens
- (BOOL)gestureRecognizer:(UIGestureRecognizer *)gestureRecognizer shouldRecognizeSimultaneouslyWithGestureRecognizer:(UIGestureRecognizer *)otherGestureRecognizer {
// Recognize simultaneously only if the other gesture is RN Screen's pan gesture (one that is used to perform fullScreenGestureEnabled)
if (gestureRecognizer == self.panGestureRecognizer && [NSStringFromClass([otherGestureRecognizer class]) isEqual: @"RNSPanGestureRecognizer"]) {
UIPanGestureRecognizer* panGestureRecognizer = (UIPanGestureRecognizer*) gestureRecognizer;
CGPoint velocity = [panGestureRecognizer velocityInView:self];
BOOL isLTR = [self isLtrLayout];
BOOL isBackGesture = (isLTR && velocity.x > 0) || (!isLTR && velocity.x < 0);
if (self.currentIndex == 0 && isBackGesture) {
self.scrollView.panGestureRecognizer.enabled = false;
} else {
self.scrollView.panGestureRecognizer.enabled = self.scrollEnabled;
}
return YES;
}
self.scrollView.panGestureRecognizer.enabled = self.scrollEnabled;
return NO;
}
} | ||
|
||
// Resetting here to default in case we add more recognizer. | ||
self.scrollView.panGestureRecognizer.enabled = self.scrollEnabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LOC will be unreachable as of now as we added only one recognizer. But still resetting the value for sanity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @intergalacticspacehighway
Thank you for adding a new feature for the Pager 🔥 Could you add the same implementation for a fabric here: https://github.com/callstack/react-native-pager-view/blob/master/ios/Fabric/RNCPagerViewComponentView.mm . Unfortunately, we have 2 implementations (paper and fabric).
"@react-navigation/native": "^5.8.10", | ||
"@react-navigation/native-stack": "5.0.4", | ||
"@react-navigation/stack": "^5.12.8", | ||
"@react-navigation/native": "^6.0.16", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we blindly bump a major version of React Navigation ? Do we have to do a migration ?
Any plan to continue work/update this to the current master? There has been a lot of activity in the repo, and I'm hopeful that it might get merged this time. One additional question, would it also allow going back from page 2 in the pager (and not just the first one), for example? If the user starts swiping from the edge? |
I think Nishan did not continue here because they started rewriting to ScrollView. @intergalacticspacehighway right ? |
Closing this in favor of #705! |
Summary
Test Plan
fullScreenGestureEnabled
on react navigation's native stack and try to swipe when the first page is reached in pager view.Screen.Recording.2022-12-03.at.4.51.52.PM.mov
Test with RTL enabled
I18nManager.forceRTL(true);
Screen.Recording.2022-12-03.at.4.48.26.PM.mov
What's required for testing (prerequisites)?
fullScreenGestureEnabled
in react navigation screen optionsWhat are the steps to reproduce (after prerequisites)?
swipe to go back
withfullScreenGestureEnabled
should work on the first page.Compatibility
Checklist
README.md