-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Virtual detector native gestures #3765
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
base: next
Are you sure you want to change the base?
Conversation
...ler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerDetectorView.kt
Outdated
Show resolved
Hide resolved
...ler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerDetectorView.kt
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/v3/NativeDetector/HostGestureDetector.web.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/apple/RNGestureHandlerDetector.mm
Outdated
Show resolved
Hide resolved
...ler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerDetectorView.kt
Outdated
Show resolved
Hide resolved
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'm curious, why did the order of attach/detach change?
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.
If you follow the logic in updateLogicDetector closely you find out that if a specific handler was previously attached to another view, which was then detached (like in the example) - due to how handlers are stored - they would first be attached to the new view and then detached completely while looping through logicChildrenToDetach. I thought that it also happens in attachHandlers, but after careful testing it is not the case. I also fixed it on android today. For some reason I can't replicate it on web, I have to dig into that.
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'm not sure what is updateLogicDetector.
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 meant updateLogicChildren, sorry
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 could not replicate the same issue on web due to how views are reused there, but I decided to add a similar fix to be safe.
packages/react-native-gesture-handler/apple/RNGestureHandlerDetector.mm
Outdated
Show resolved
Hide resolved
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.
Please also merge the next branch
|
|
||
| return () => { | ||
| if (tag != null) { | ||
| unregister(viewTag); |
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.
| unregister(viewTag); | |
| unregister(tag); |
I assume this shouldn't be unregistering for the old view tag.
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.
Thanks, c06d53e.
342c019 to
c06d53e
Compare
Description
This PR makes
VirtualDetectorcompatible with native gestures. Back in #3689 I decided to handleVirtualDetectorcompatibility with native gestures for later, as it is a extremely niche feature and it seemed to require a lot of work. Turns out it doesn't, beacause we use wrap in VirtualDetector we can handle them as normal gestures.Test plan
Tested on: