-
Notifications
You must be signed in to change notification settings - Fork 63
Reanimated #151
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: thomas/no-use-before-define
Are you sure you want to change the base?
Reanimated #151
Conversation
6251862 to
8e8b26f
Compare
8e8b26f to
fa598a6
Compare
afd207c to
aaa96cc
Compare
# Conflicts: # src/ReactNativeZoomableView.tsx
aaa96cc to
9945d2b
Compare
…and switch from using touch.absoluteX/Y to touch.x/y
elliottkember
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.
This is looking really great — amazing work. The performance gains from things like worklets will be out of this world.
We may need to make some minor changes in our app to implement this but they should be small fry compared to these changes.
| disablePanOnInitialZoom?: boolean; | ||
|
|
||
| // Zoom animated value ref | ||
| zoomAnimatedValue?: Animated.Value; |
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.
This will probably be the tricky part in our internal implementation of this. I've never truly liked the way this component takes an animated value as a prop, this is how we ended up with sheet zoom contexts where other components need to know about zoom as it happens and we needed an HOC with the animated zoom value.
It seems like an imperative handle is probably the way to do this, plus maybe some built-in components for handling interpolations — or perhaps just a context provided by this component to its children.
src/ReactNativeZoomableView.tsx
Outdated
|
|
||
| const distance = calcGestureTouchDistance(e); | ||
|
|
||
| // TODO this gets called way too often, we need to find a better way |
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.
Yeah these kind of props sort of break the whole magic of reanimated and animated values huh?
Do we use this, or can we just subscribe to the animations and react with animated components?
I think this will maybe just be a huge major version release with lots of breaking changes that remove footguns
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.
Comment @cursor review or bugbot run to trigger another review on this PR
|
bugbot run |
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.
✅ Bugbot reviewed your changes and found no bugs!
Comment @cursor review or bugbot run to trigger another review on this PR
|
I noticed that the example and base repos have different versions of reanimated. This is a good thing, I think — it shows that the library works on both versions. But when I went to refactor Might be worth using the same version in both... or maybe even having two example apps for testing both reanimateds at once?? |
This is a new worklet function in Reanimated 4 that they want us to use <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Updates the example to use the new worklet scheduling utility. > > - In `example/App.tsx`, replace `runOnJS(debouncedUpdateMovePin)(position)` with `scheduleOnRN(debouncedUpdateMovePin, position)` inside `onStaticPinPositionMoveWorklet` > - Add `scheduleOnRN` import from `react-native-worklets` and remove `runOnJS` import from `react-native-reanimated` > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit dae0226. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
| <Button | ||
| // Toggle modal to test if zoomable view works correctly in modal, | ||
| // where pull-down-to-close gesture can interfere with pan gestures. | ||
| title={`Toggle Modal Mode`} | ||
| onPress={() => { | ||
| setModal((value) => !value); | ||
| }} | ||
| /> | ||
| </Wrapper> |
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.
Clever! I think this should exhibit the same behaviour as the react-navigation / react-native-screens pull to close, but I might add the routing library and some formSheet / pageSheet routes just to test the theory
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.
Reanimated Context
…ly does nothing except document)

Reanimated implementation is here and now with almost all features preserved. Some features like pin onPress/onLongPress need to be stripped due to time constraint.
Note
Migrates the zoomable view to a Reanimated + Gesture Handler implementation with worklet-driven callbacks and smoother animations; updates the example app and tooling to support the new stack.
onTransformWorkletandonStaticPinPositionMoveWorklet, preserves double-tap/pinch/shift behavior, and refines static pin updatesModalwrapper, removes oldzoomAnimatedValuereact-native-reanimated/gesture-handler/worklets; minor style/tsconfig tweaksWritten by Cursor Bugbot for commit 490ae19. Configure here.