Skip to content
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

fix: onGestureEnd -> endWithSpring uses outdated data via useSharedValue #574

Merged
merged 1 commit into from
May 6, 2024

Conversation

nmassey
Copy link
Contributor

@nmassey nmassey commented Mar 23, 2024

What: the bug

When endWithSpring is called, it is using the previous values for scrollEndVelocity and scrollEndTranslation (rather than the values just set in onGestureEnd). This seems to cause strange carousel behavior for the user: the function isn't using the values from the most recent pan.

From the user's standpoint, the problem is very obvious (and potentially frustrating) when the user pans one direction, then reverses direction for their next pan. (Please see video demonstration below.)

Why

This is due to updates to shared values created via useSharedValue() updating asynchronously (see https://docs.swmansion.com/react-native-reanimated/docs/core/useSharedValue#remarks ).

What: proposed fix

Rather than relying on the shared data variable between function calls, let's explicitly pass the values as arguments to endWithSpring.

May resolve the following issues

  1. First swipe in direction is reverted in v4.0.0-alpha.9 #535
  2. Page rendering is incorrect when changing the scrolling direction. #568
  3. first slide(first index) and last side (last index) unable to swipe on the first tap need to tap two times then it is working and middle slides working normal using "react-native-reanimated-carousel": "^4.0.0-alpha.10" version #579

Screengrab video recordings

buggy behavior on 4.0.0-alpha.10 (unpatched)

Notice, panning the same direction works OK, but panning alternate direction fails.

Screen.Recording.2024-03-25.at.09.19.01.mov

improved behavior with patch

Screen.Recording.2024-03-25.at.09.24.18.mov

Copy link

changeset-bot bot commented Mar 23, 2024

⚠️ No Changeset found

Latest commit: 86da6ac

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Mar 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-reanimated-carousel ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 23, 2024 6:39am

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 23, 2024
"worklet";
const origin = translation.value;
const velocity = scrollEndVelocity.value;
const velocity = scrollEndVelocityValue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I'm explicitly leaving both the function parameter scrollEndVelocityValue and this local const velocity.
We could (should?) eliminate this line, though, and instead rename the function parameter to velocity instead.

@nmassey nmassey changed the title fix: onGestureEnd -> endWithSpring uses outdated data via useSharedValue fix: onGestureEnd -> endWithSpring uses outdated data via useSharedValue Mar 25, 2024
@vricosti
Copy link

When will it be released ? What is missing ?

@sieeniu
Copy link

sieeniu commented May 6, 2024

when it would be merged?

@dohooo dohooo changed the base branch from main to dev May 6, 2024 09:46
@dohooo dohooo merged commit 9f14e92 into dohooo:dev May 6, 2024
2 checks passed
@dohooo
Copy link
Owner

dohooo commented May 6, 2024

@nmassey Thanks! #601

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants