Skip to content

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

Merged
merged 25 commits into from
Sep 29, 2021
Merged

Feat/add transition animator #1479

merged 25 commits into from
Sep 29, 2021

Conversation

M-i-k-e-l
Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l commented Aug 18, 2021

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' (or top and bottom), 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 of HiddenLocation (I've renamed it to TransitionLocation in TransitionView).

A few other things:

  1. There's a question (a TODO) about how to handle opacity from the user.
  2. We can allow the user to add his\her own animation. Edit: I probably need to select a different animation (time probably) for this, I don't want to block this PR with this though, it'll be easier to test with future PRs (Dialog?).
  3. The usage of TransitionLocation in the screen is a bit clunky, not sure if I did something wrong or if using a union is less usable than an enum, WDYT?

Changelog

Add TransitionView - a wrapping view that adds enter\exit animation (incubator).

Copy link
Collaborator

@ethanshar ethanshar left a 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)

@M-i-k-e-l M-i-k-e-l requested a review from ethanshar September 1, 2021 18:00
@M-i-k-e-l
Copy link
Collaborator Author

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},
Copy link
Collaborator

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) ?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@M-i-k-e-l M-i-k-e-l requested a review from ethanshar September 14, 2021 07:00
@M-i-k-e-l M-i-k-e-l requested a review from ethanshar September 14, 2021 13:03
@ethanshar ethanshar merged commit 4ac8033 into master Sep 29, 2021
@M-i-k-e-l M-i-k-e-l deleted the feat/add-transition-animator branch October 4, 2021 08:00
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.

2 participants