-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Add runOnJS to config
#3743
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
Add runOnJS to config
#3743
Conversation
MatiPl01
left a comment
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.
LGTM, left just 2 small questions
| ); | ||
| } | ||
|
|
||
| // This has to be done ASAP as other hooks depend `shouldUseReanimated`. |
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.
Is this comment still valid? After you removed the shouldUseReanimated prop?
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 think it's still needed since shouldUseReanimated became shouldUseReanimatedDetector, but it definitely needs to be updated.
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.
It is valid, changed to shouldUseReanimatedDetector in 7a4a014
| config.needsPointerData = shouldHandleTouchEvents(config); | ||
| config.dispatchesAnimatedEvents = isAnimatedEvent(config.onUpdate); | ||
| config.dispatchesReanimatedEvents = | ||
| config.shouldUseReanimatedDetector && runOnJS !== true; |
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 it can be extracted to a helper function. The similar logic is already in the packages/react-native-gesture-handler/src/v3/hooks/utils/reanimatedUtils.ts
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 if that makes sense - one value comes from config, other from shared value listener. Extracting it into a separate function would make it more complicated (like using maybeUnpackValue inside), so I don't think it makes much sense.
Though I've unified checks in b470764 😅
...ve-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandler.kt
Show resolved
Hide resolved
| ); | ||
| } | ||
|
|
||
| // This has to be done ASAP as other hooks depend `shouldUseReanimated`. |
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 think it's still needed since shouldUseReanimated became shouldUseReanimatedDetector, but it definitely needs to be updated.
Description
This PR adds
runOnJSto gesture config.Test plan
Tested on the following example: