-
Notifications
You must be signed in to change notification settings - Fork 734
Feat/add transition animator #1479
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
Conversation
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.
See my comments.
I mostly focused on readability and code structure.
I'm curious if this API will fit to our use cases.
(I mean the imperative API over the declarative one)
I've renamed and refactored a few things, let me know if anything else comes to mind. |
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
[]); | ||
|
||
const animate = useCallback((to: {x: number; y: number}, |
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.
The arguments order here feels a little weird.
You have to
the end callback and then the direction..
Don't you think it's more appropriate to have (to, direction, endCallback) ?
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.
Yes, but the direction can be undefined and A required parameter cannot follow an optional parameter.
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.
Why is direction
argument undefined?
In animate
implementation, the method does nothing if direction was not passed...
Also, in all usages, you always pass the direction.
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 guess you're right, fixed.
Depends on
#1475
Description
Add
TransitionView
- a wrapping view that adds enter\exit animation (incubator).I know there's a few
location === 'left' || location === 'right'
(ortop
andbottom
), but to move it to a method I'll need to have two versions (worklet
and regular), and I'll have to add!
in some places.I'm unsure if we can use
useHiddenLocations
elsewhere, and about the naming ofHiddenLocation
(I've renamed it toTransitionLocation
inTransitionView
).A few other things:
opacity
from the user.TransitionLocation
in the screen is a bit clunky, not sure if I did something wrong or if using aunion
is less usable than an enum, WDYT?Changelog
Add
TransitionView
- a wrapping view that adds enter\exit animation (incubator).