[Drawer] refactor: use GestrueDetector to fix onStart bug #10928
[Drawer] refactor: use GestrueDetector to fix onStart bug #10928hannojg wants to merge 14 commits intoreact-navigation:7.x-oldfrom
Conversation
BREAKING CHANGE: Previously, you could specify a route `key` to navigate to, e.g.
`navigation.navigate({ key: 'someuniquekey' })`. It's problematic since `key` is an internal
implementation detail and created by the library internally - which makes it weird to use. None
of the other actions support such usage either.
In addition, we have already added a better API (`getId`) which can be used for similar use
cases - and gives users full control since they provide the ID and it's not autogenerated by the
library.
So this change removes the `key` option from the `navigate` action.
`navigate` BREAKING CHANGE: Previously, `navigate` method navigated back if the screen already exists in the stack. I have seen many people get confused by this behavior. This behavior is also used for sending params to a previous screen in the documentation. However, it's also problematic since it could either push or pop the screens based on the scenario. This removes the going back behavior from `navigate` and adds a new method `popTo` to go back to a specific screen in the stack. The methods now behave as follows: - `navigate(screenName)` will stay on the current screen if the screen is already focused, otherwise push a new screen to the stack. - `popTo(screenName)` will go back to the screen if it exists in the stack, otherwise pop the current screen and add this screen to the stack. - To achieve the previous behavior with `navigate`, you can use the `getId` prop in which case it'll go to the screen with the matching ID and push or pop screens accordingly.
BREAKING CHANGE: Previously, the `onReady` prop and `navigationRef.isReady()` work slightly differently. The `onReady` callback fired when `NavigationContainer` finishes mounting and deep links is resolved. The `navigationRef.isReady()` method additionally checks if there are any navigators rendered - which may not be `true` if the user is rendering their navigators conditionally inside a `NavigationContainer`. This changes `onReady` to work similar to `navigationRef.isReady()`. The `onReady` callback will now fire only when there are navigators rendered - reflecting the value of `navigationRef.isReady()`.
PR removes deprecation warnings introduced in react-navigation@8e0ae5f
…t-navigation#10905) Due to backward compatibility reasons, React Navigation 5 and 6 support navigating to a screen in a child navigator with `navigation.navigate(ScreenName)` syntax. But this is problematic with the new architecture - it only works if the navigator is already mounted, doesn't work with TypeScript, etc. That's why there's a special API to navigate to a nested screen (`navigation.navigate(ParentScreenName, { screen: ScreenName })`). This drops this behavior and adds a prop to explicitly enable it to make it easier to migrate. This prop will be removed in the next major.
✅ Deploy Preview for react-navigation-example ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 7.x #10928 +/- ##
======================================
Coverage ? 75.56%
======================================
Files ? 171
Lines ? 5127
Branches ? 1987
======================================
Hits ? 3874
Misses ? 1213
Partials ? 40 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
satya164
left a comment
There was a problem hiding this comment.
Thanks for the PR! However, this can't be merge to current stable version since it's a breaking change - as GestureDetector is only available from gesture handler 2.x.
Can you target the 7.x branch with PR instead? In addition, we need to bump the minimum peer dependency version on gesture handler across all packages in the repo. For drawer, we also need to do this change for the legacy implementation as well.
a5ce8cd to
782296f
Compare
782296f to
9788b1b
Compare
| <DrawerProgressContext.Provider value={progress}> | ||
| <PanGestureHandler | ||
| activeOffsetX={[-SWIPE_DISTANCE_MINIMUM, SWIPE_DISTANCE_MINIMUM]} | ||
| failOffsetY={[-SWIPE_DISTANCE_MINIMUM, SWIPE_DISTANCE_MINIMUM]} |
There was a problem hiding this comment.
I think you missed this prop during the migration
| // @ts-expect-error Ref types incompatible yet, needs to get fixed upstream | ||
| panGesture.config = { | ||
| ...panGesture.config, | ||
| ...gestureHandlerProps, |
There was a problem hiding this comment.
Hmm, this raises some more problems.
Currently, gestureHandlerProps allows users to customise PanGestureHandler simply by passing props to it (with React.ComponentProps<typeof PanGestureHandler> signature).
After some consultation with @j-piasecki we've come to conclusion there's no guarantee whatsoever that props coming from v1 are going to work with RNGH v2. Or maybe the other way around - there are for sure going to be more props in RNGH 2.
We would have to adjust the API on how users can customize gestures in React Navigation v7.
The solution @j-piasecki and I propose is to drop the gestureHandlerProps prop in favor of the panGesture prop. Users would be able to pass Gesture.Pan() with options the user wants to add or override. This then would have to be merged right in this selected line.
There was a problem hiding this comment.
oh yeah def, that sounds like a clean API. Shall I make that change in the props, or will you do at some point in the 7.x branch?
534aa0c to
1b0a943
Compare
|
@hannojg Thanks. I think we would like to proceed with that PR. Is there a chance you can update the branch and make it up-to-date with the main (which serves as 7.x)? |
|
Closing in favor of #11776 |
|
Hey! This issue is closed and isn't watched by the core team. You are welcome to discuss the issue with others in this thread, but if you think this issue is still valid and needs to be tracked, please open a new issue with a repro. |
Motivation
In #10927 I showed how
onStartgets called although the activation criteria isn't met. This causes issues.This PR fixes the issue by migrating to the new
GestureDetectorAPI which has a separateonBeginandonStartmethod, whereonStartwill only be called if the activation criteria is really met (see here software-mansion/react-native-gesture-handler#1225)Fixes #10927
Test plan
console.loginonStartinDrawer.tsxexample/src/index.tsxadd to the DrawerscreenOptions.swipeEdgeWidth: 500,onStart, although the user just taped and the activation criteria wasn't met