[ReanimatedSwipeable] Multiple bug fixes and improvements
#3149
Conversation
…elocity to the config
…he row state" This reverts commit 866425d.
Is it necessary to fix all of them in just 1 PR? I'd prefer to split it into smaller ones if possible. |
Most of them are very minor, most also depend on each other / are fixes to the same part of I can split them up if you'd prefer that but i think it'd significantly complicate the process, i'll definitely have to deal with conflicts, and PRs blocking eachother. |
## Description Add missing documentation for `onSwipeableOpenStartDrag` and `onSwipeableOpenStartDrag` callbacks of `ReanimatedSwipeable` Moved from #3149
…pdate (#3151) ## Description Fix `onStart` callbacks of `ReanimatedSwipeable` being called every update. Moved from #3149 ## Test plan - open `Swipeable Reanimation` example - replace the `ReanimatedSwipeable` component with the following code: ```js <ReanimatedSwipeable containerStyle={styles.swipeable} onSwipeableOpenStartDrag={(direction) => console.log(direction)} onSwipeableCloseStartDrag={(direction) => console.log(direction)}> <Text>Swipeable!</Text> </ReanimatedSwipeable> ``` - see how only one update is sent to the console, while before this PR new updates were constantly generated
| appliedTranslation.value = withSpring( | ||
| toValue, | ||
| translationSpringConfig, | ||
| (isFinished) => { | ||
| if (isFinished) { | ||
| runOnJS(dispatchEndEvents)(fromValue, toValue); | ||
| runOnJS(dispatchEndEvents)(frozenRowState, toValue); |
There was a problem hiding this comment.
Are all events executed on the JS thread?
There was a problem hiding this comment.
I believe all of them are, couldn't find any that aren't
There was a problem hiding this comment.
I think we can update the calling logic to check whether the callback is a worklet and run it on the UI thread if it is (that's for another PR though).
There was a problem hiding this comment.
Great idea, will do that.
There was a problem hiding this comment.
I still think it could be split into smaller PRs (like switching to enum instead of string), but I won't block it. Please wait for @j-piasecki final opinion on this
| enum SwipeDirection { | ||
| LEFT = 'left', | ||
| RIGHT = 'right', | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd move this declaration at the top of the file (somewhere around SwipeableExcludes type). I know it is JS and it works, but I think it's a good practice. Also if you read code from the top you know what SwipeDirection is (at first I thought it was a number, then I saw the declaration)
There was a problem hiding this comment.
I think we can merge this, the changes are not necessarily related but the PR isn't that big. In the future though, please try to open separate PRs for unrelated changes (like extracting direction to enum).
Please address #3149 (comment) first.
Description
This PR applies multiple bug fixes and improvements to the
ReanimatedSwipeableChanges:
openandwillOpencallbacks.startDragcallbacks receiving invaliddirectionwhen opening swipeable.closeandwillClosecallbacks.onSwipeableWillCloseandonSwipeableClosecallbacks sometimes being called on open.progressvalue skipping last10%of it's range and having a delay relative to thetranslation.closes ReanimatedSwipable Delayed #3147
Test plan
Will create an example showcasing outputs of all the affected callbacks, work in progress.