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 velocity regression & bug #798

Merged
merged 10 commits into from Aug 15, 2022
Merged

Fix velocity regression & bug #798

merged 10 commits into from Aug 15, 2022

Conversation

wcandillon
Copy link
Collaborator

@wcandillon wcandillon commented Aug 12, 2022

I've made a small type change so that the original bug and current regression would have been caught by TypeScript (TouchInfo | undefined).

I also added a test case that outlines a bug which when fixed, make the velocity finally feel natural.

fixes #800

@wcandillon
Copy link
Collaborator Author

wcandillon commented Aug 12, 2022

@chrfalch I added a failing test case that may be a bug: onActive is called an extra time when onEnd. Fixing it seem to make it obvious that now the values are correct, the behaviour feels much more natural.

@wcandillon wcandillon changed the title Fix velocity regression Fix velocity regression & bug Aug 12, 2022
@wcandillon
Copy link
Collaborator Author

I can remove the extra onActive call but then can we add tests for potential regressions this may cause?
Do we call onActive before onEnd for tap touches?

@chrfalch
Copy link
Collaborator

I can remove the extra onActive call but then can we add tests for potential regressions this may cause?
Do we call onActive before onEnd for tap touches?

The reason for the extra onActive is that we're trying to ensure that at least one onActive call is emitted - if you tap you could end up with only start/end.

But if this is not a valid use case and there are issues with the extra onActive (velocity?), I'm find with removing it.

@chrfalch
Copy link
Collaborator

Awesome cleanup 🙏

@wcandillon
Copy link
Collaborator Author

On gesture handler only onEnd is invoked, for instance: https://github.com/wcandillon/can-it-be-done-in-react-native/blob/ed6211f1b93f4502722e9d209df861f1549c895f/reanimated-2/src/PizzaChallenge/components/IngredientSelection.tsx#L59.

If this is the use reason of the extra call, I would be in favor of removing it. We should add more unit test for the touch handler.

@chrfalch
Copy link
Collaborator

On gesture handler only onEnd is invoked, for instance: https://github.com/wcandillon/can-it-be-done-in-react-native/blob/ed6211f1b93f4502722e9d209df861f1549c895f/reanimated-2/src/PizzaChallenge/components/IngredientSelection.tsx#L59.

If this is the use reason of the extra call, I would be in favor of removing it. We should add more unit test for the touch handler.

The use case is to ensure at least one onActive call - but I agree that it isn't necessary super valid so we can remove. Go from me!

@wcandillon
Copy link
Collaborator Author

@chrfalch this is ready for review.

@@ -75,17 +75,17 @@
}
},
"devDependencies": {
"@types/jest": "^27.0.3",
"@types/jest": "^28.1.6",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a specific reason for upgrading Jest here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is so we can mark tests as expected failures.

@wcandillon wcandillon merged commit 758ac4a into main Aug 15, 2022
@wcandillon wcandillon deleted the fix-velocity-regression branch August 15, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined is not an object (evaluation 'prevVelocityRef.current[touch.id].x') when running Examples
2 participants