Skip to content

[Drawer] refactor: use GestrueDetector to fix onStart bug #10928

Closed
hannojg wants to merge 14 commits intoreact-navigation:7.x-oldfrom
hannojg:fix/refactor-to-gesture-detector-api
Closed

[Drawer] refactor: use GestrueDetector to fix onStart bug #10928
hannojg wants to merge 14 commits intoreact-navigation:7.x-oldfrom
hannojg:fix/refactor-to-gesture-detector-api

Conversation

@hannojg
Copy link
Copy Markdown
Contributor

@hannojg hannojg commented Oct 14, 2022

Motivation

In #10927 I showed how onStart gets called although the activation criteria isn't met. This causes issues.
This PR fixes the issue by migrating to the new GestureDetector API which has a separate onBegin and onStart method, where onStart will only be called if the activation criteria is really met (see here software-mansion/react-native-gesture-handler#1225)

Fixes #10927

Test plan

  • Set a console.log in onStart in Drawer.tsx
  • In example/src/index.tsx add to the Drawer screenOptions.swipeEdgeWidth: 500,
  • Now press anywhere in the example app
  • Without these changes you will read onStart, although the user just taped and the activation criteria wasn't met
  • With these changes this won't happen, only when the user gesture met the criteria

satya164 and others added 7 commits October 11, 2022 14:39
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()`.
…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.
@netlify
Copy link
Copy Markdown

netlify bot commented Oct 14, 2022

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit a5ce8cd
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/634a565bdb273c000a4332d1
😎 Deploy Preview https://deploy-preview-10928--react-navigation-example.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 15, 2022

Codecov Report

❗ No coverage uploaded for pull request base (7.x@e67d2c7). Click here to learn what that means.
Patch has no changes to coverable lines.

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Copy Markdown
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

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.

@hannojg hannojg changed the base branch from main to 7.x October 17, 2022 06:52
@hannojg hannojg force-pushed the fix/refactor-to-gesture-detector-api branch from a5ce8cd to 782296f Compare October 17, 2022 07:20
@hannojg hannojg force-pushed the fix/refactor-to-gesture-detector-api branch from 782296f to 9788b1b Compare October 17, 2022 07:21
@hannojg
Copy link
Copy Markdown
Contributor Author

hannojg commented Oct 17, 2022

Hey @satya164, i just:

  • Changed the base to 7.x and rebased the branch onto it
  • Updated the peerDependency of RNGH to be >= 2.0.0
  • I also wanted to update the legacy implementation, but as mentioned here, the legacy implementation is already working correctly

<DrawerProgressContext.Provider value={progress}>
<PanGestureHandler
activeOffsetX={[-SWIPE_DISTANCE_MINIMUM, SWIPE_DISTANCE_MINIMUM]}
failOffsetY={[-SWIPE_DISTANCE_MINIMUM, SWIPE_DISTANCE_MINIMUM]}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@satya164 satya164 force-pushed the 7.x branch 3 times, most recently from 534aa0c to 1b0a943 Compare November 16, 2022 12:15
@satya164 satya164 changed the base branch from 7.x to 7.x-old November 28, 2022 10:02
@osdnk
Copy link
Copy Markdown
Member

osdnk commented Sep 4, 2023

@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)?

@satya164
Copy link
Copy Markdown
Member

Closing in favor of #11776

@satya164 satya164 closed this Jan 18, 2024
@github-actions
Copy link
Copy Markdown

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.

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.

[Drawer] High swipeEdgeWidth value causes keyboard to gets constantly closed

5 participants