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

Navigator: add support for exit animation #64777

Open
wants to merge 34 commits into
base: trunk
Choose a base branch
from

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Aug 24, 2024

What?

Part of #59418
Requires #64786 to be merged first

Add exit animations to Navigator screen

Why?

An exit animation can help the user understand the transition to a new screen better — and gives an additional sense of polish to the UI.

The animation specs (durations, delays, easing functions) are also updated following the latest motion guidelines (cc @nick-a8c)

How?

  • Added a useScreenAnimatePresence hook which takes care of managing the screen's animation lifecycle
  • Use CSS transitions to animate the screen
  • Use data- attributes to set animation state on the DOM element, which is used to trigger the CSS transitions
  • Use CSS grid on the provider to force all screens to overlap, while also pushing the height of the container
  • Always render the screen root element, while conditionally rendering its contents
  • Updated tests to completely skip animations, that were causing errors and general flakiness
  • Added a note in the README about setting the wrapper height (related conversation in Navigator: polish Storybook examples #64798 (comment))

Testing Instructions

  • Make sure that the animations are always applied to the pair of screens (the one transitioning in and the one transitioning out), both when navigating to a new screen and back to the parent screen;
  • Make sure that the animations work as expected on all supported browsers;
  • Make sure that the animations are not performed when enabling "reduced motion" browser/OS settings

Testing Instructions for Keyboard

Screenshots or screencast

navigator-exit-animations.mp4

10x slow-motion:

navigator-exit-animations-slowmo.mp4

@ciampo ciampo self-assigned this Aug 25, 2024
@ciampo ciampo requested review from a team August 25, 2024 14:55
@ciampo ciampo added [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. labels Aug 25, 2024
@ciampo ciampo changed the title Navigator: exit animation Navigator: add support for exit animation Aug 25, 2024
@ciampo ciampo marked this pull request as ready for review August 25, 2024 14:59
@ciampo ciampo requested a review from ajitbohra as a code owner August 25, 2024 14:59
Copy link

github-actions bot commented Aug 25, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@jasmussen
Copy link
Contributor

Nice. Is it possible to test this in the site editor? It's not clear that the new animation from this branch is applied there:

nav

Here's trunk for comparison:

trunk

@ciampo
Copy link
Contributor Author

ciampo commented Aug 26, 2024

It appears that the site editor sidebar doesn't use Navigator anymore — looking at the file history, the change happened in #60466. I was on parental leave at the time, but it seems like the main reason was to use the global site-editor router instead of having to coordinate with Navigator's internal router.

Navigator is still being used in the Global Styles sidebar, though.

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Animating presence will likely be a recurring need. How can we abstract this into something encapsulated and reusable, so we don't have to add in a bunch of ad hoc animation coordinating logic into a component every time we need an entry/exit animation?

Not immediately sure if we should use a library like react-transition-group (maybe 1.7kB gzipped if we only needed the Transition component), or if we want to maintain our own for an even smaller size. Thoughts?

@ciampo
Copy link
Contributor Author

ciampo commented Aug 27, 2024

Animating presence will likely be a recurring need

That's true, although in many cases, animating presence can be done with some recent CSS features. But those can't be used with Navigator because we remove a Screen's markup when the screen gets deselected.

Not immediately sure if we should use a library like react-transition-group (maybe 1.7kB gzipped if we only needed the Transition component), or if we want to maintain our own for an even smaller size.

I'm conflicted — on one end, I don't like to reinvent the wheel if the problem has been solved well by third-parties. On the other, we're trying to move away from framer-motion for smaller bundle size and better performance (using CSS native features as much as possible).

If we used react-transition-group, we'd need to use the CSSTransition component (2.6kB), since the Transition component is just the abstract logic.

Also, this specific type of animation will be much easier when view transitions become more advanced and better supported in the next few years.

How can we abstract this into something encapsulated and reusable, so we don't have to add in a bunch of ad hoc animation coordinating logic into a component every time we need an entry/exit animation?

I wonder if doing so would represent a form of early optimization. Maybe we can keep Navigator as-is, and re-discuss this topic if/when we have more real use cases for animating presence, so that we can better understand what type solution we'd need to implement?

@ciampo ciampo requested a review from mirka August 28, 2024 21:57
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

I wonder if doing so would represent a form of early optimization. Maybe we can keep Navigator as-is, and re-discuss this topic if/when we have more real use cases for animating presence, so that we can better understand what type solution we'd need to implement?

I'm not familiar enough to make a confident judgement in either direction, but I trust you on your assessment after working on it.

I guess what immediately makes me a bit uncomfortable though is how the animation logic is interspersed in the component code. Do you think it would be worth the effort to extract most or all of the animation logic into a single hook? No regard for reusability at the moment, but just to isolate all the animation concerns into a single place. I'm already a bit afraid of touching this code, and I think separating the concerns would help a lot to ease that sense of complexity.

@ciampo
Copy link
Contributor Author

ciampo commented Aug 28, 2024

Rebased to include all recent Navigator changes. I will look at addressing the remaining feedback by the end of the week

@ciampo ciampo force-pushed the feat/navitator-animation branch 2 times, most recently from 7f2a8e4 to 68d898a Compare August 29, 2024 10:07
@ciampo
Copy link
Contributor Author

ciampo commented Aug 29, 2024

I guess what immediately makes me a bit uncomfortable though is how the animation logic is interspersed in the component code. Do you think it would be worth the effort to extract most or all of the animation logic into a single hook? No regard for reusability at the moment, but just to isolate all the animation concerns into a single place.

This is a great suggestion, I definitely agree. I pushed a couple of commits and things are looking much more clear :)

@ciampo ciampo requested a review from mirka August 29, 2024 10:11
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

I still need to test and review this thoroughly, but I'm submitting some early, naive feedback.

packages/components/src/navigator/test/index.tsx Outdated Show resolved Hide resolved
- more clear animation status names
- apply CSS animation only while effectively animating
- fix bug in the animationEnd callback which was matching animation end
  events too loosely, thus causing glitches
This reverts commit 946f9c953232b788f58050d2a94d9d131527a180.
@ciampo
Copy link
Contributor Author

ciampo commented Sep 20, 2024

With #65494 merged, the glitch reported above should be fixed (thank you @jsnajdr 🙏 )

I rebased, restored the CSS animation- based approach, and added a few tweaks (like moving to data-* attributes, instead of dynamic emotion styles).

I'm going to extract changes not related to the exit animation to separate PRs (#65522, #65523, #65527), but in the meantime, this PR is ready to be tested again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants