-
-
Notifications
You must be signed in to change notification settings - Fork 514
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 full width swipe option #1072
Conversation
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.
Looks good, left a couple minor comments inline.
createNativeStackNavigator/README.md
Outdated
|
||
#### `gestureEnabled` | ||
#### `fullWidthGestureEnabled` (iOS only) |
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.
Maybe rename to sth like fullScreenSwipeEnabled
– "width" seem like a weird choice of word as it applies to the area. Also "gesture" is a term that we use for things like sliding down modals, so maybe "swipe" is a better word as it suggest that this only applies to swiping left?
ios/RNSScreenStack.m
Outdated
// we want only `RNSPanGestureRecognizer` to be able to recognize when | ||
// `fullWidthGestureEnabled` is set and only then |
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.
rephrase
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.
and only then
is not necessary since there is a comment about it already: https://github.com/software-mansion/react-native-screens/pull/1072/files#diff-1c3e13a1b0dcbaa5f7cff5f3ce12cc915fb509725faf693fb65cd23ceb49c4a9R595, so I will remove it.
ios/RNSScreenStackAnimator.m
Outdated
} else { | ||
[self animateSimplePushWithTransitionContext:transitionContext toVC:toViewController fromVC:fromViewController]; | ||
} | ||
return; |
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 personally try to avoid early returns if there exists a more logical structure of the conditions. It appears like it'd suffice to put the remainig code in else
block here and that'd result in the same behavior w/o a need of using return
here
Is this supposed to work in @react-navigation/native-stack? |
I think that since https://github.com/react-navigation/react-navigation/blob/main/packages/native-stack/src/views/NativeStackView.native.tsx#L176 does not spread all of the props, all of the new ones need to be added there explicitly, so we need new version of |
Any updates on this @WoLewicki ? |
Are you asking about v6 version of this? If so, I am afraid there is still no PR with this option there. |
I'd love to help out, is the PR simply adding fullWidthGestureEnabled as a prop to NativeStackView.native.tsx and then passing it down into Screen? |
Yeah that's all were looking for! |
@kyang6 I believe there should not be much that needs to be done to make it work. |
Gotcha! Quick question, has fullWidthGestureEnabled been added to ScreenProps? |
Description
PR adding an option on iOS to begin the swipe gesture in any place on the screen. Since it comes with a custom gesture recognizer, the transition animation must be provided, therefore
simple_push
animation has been chosen as the closest one to the default. Requested in #251.Be aware that using this option with
gestureEnabled
set totrue
will cancel JS recognizers on the screen when using vertical or horizontal swipe gesture.Changes
Added
fullWidthGestureEnabled
prop with the described behavior.Test1072.tsx
Checklist