Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Navigator: add support for exit animation #64777
Navigator: add support for exit animation #64777
Changes from all commits
ee11150
2832ef0
e6ab671
0a869dc
85b5214
7b2df6a
9792e28
12a185f
668a222
98c7b20
808780e
1e0ae6b
4a19bb1
5dde128
ea5f1ed
c9e9998
205b9aa
9931a50
c98ba81
d572679
80bddac
2900c46
56a1cb9
396e3f3
ec1d6a7
effbe13
ce54ad6
759e55f
3988e43
b15f9af
d67f6f2
283dc1e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Lots of good work here, but I'm wondering if we're trying too hard not to use
framer-motion
. When it's about a simple animation, using CSS makes lots of sense. But when we need to reinvent the wheel in cases like animating presence, I feel like the best bet would be to useframer-motion
. Bundle size is important, but DX and UX are more important, so we might have to make compromises. Especially given that we're looking to build more complex motion and animations for many of the components and flows.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 definitely feel better about it after the refactor into a designated hook, but also it’s still unknown how bug-free the current code is, and how many other components will need to animate presence.
My instinct is that it could still make sense to migrate from Framer Motion to a lighter library like React Transition Group. But then again, if it's a substantial effort to migrate from FM to RTG, maybe even that isn't worth it.
I’d love to hear from @ciampo and @DaniGuardiola about how they currently assess the overall cost/benefit around these rewrites. Let's make sure the benefits are still big enough to justify our rewrite efforts and potential increase in maintenance cost.
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.
As for this particular PR, I'm personally comfortable enough with the logic isolation now to merge it as an incremental improvement over trunk. Is it scalable/sustainable? Maybe not. But it seems like we could swap it out without too much difficulty if we ultimately decide to use a library.
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 don't have particularly strong feelings about blocking this particular PR in favor of
framer-motion
, but I share the sentiment on scalability, the extra maintenance cost, and moving in that direction in the future based on the amount of custom work and wheel rediscovery we need to do. It's worth considering doing more specific measurements to ensure that a refactor away fromframer-motion
actually yields noticeable results. We should also consider the opportunity cost - what we could have built/shipped if we reusedframer-motion
instead of rediscovering it and maintaining the extra layer of complexity.