Page MenuHomePhabricator

[native] Modify overlay context to handle reanimated 2 components
ClosedPublic

Authored by inka on Aug 13 2024, 8:23 AM.
Tags
None
Referenced Files
F3203831: D13064.diff
Sat, Nov 9, 8:27 PM
F3193589: D13064.id43448.diff
Fri, Nov 8, 11:06 PM
Unknown Object (File)
Fri, Nov 8, 12:23 PM
Unknown Object (File)
Tue, Oct 29, 4:55 PM
Unknown Object (File)
Fri, Oct 18, 9:48 PM
Unknown Object (File)
Fri, Oct 18, 9:41 PM
Unknown Object (File)
Fri, Oct 18, 9:38 PM
Unknown Object (File)
Oct 7 2024, 3:32 AM
Subscribers

Details

Summary

issue: ENG-8914

Overlay context used to provide for each screen their own position - a reaniamted 1 value that was supposed to be used for animating views. It was provided by the context, so that animations could be still run when the user decided to leave the context. The idea was that afre the navigation goBackOnce function was called, Overaly navigator would start the exit animation and keep rendering the screen as long as the animation was running. Once the animation was done, the Overlay navigator would add a listener to the screen's listeners array, which would cause Animated.Code to be rendered, and the screen would get unmounted by the listener.

In reanimated 2 we take a very different approach to how we animate things, and how we handle rendering the screen as long as we need for the animation to run:

  1. Instead of animating by using a Value and interpolating it, we will be using entering and exiting animations. I consulted the reanimated team a lot in trying to figure out how to refactor our logic, and this is the approach they strongly recommended.
  2. The exiting animation can be passed a callback, that will be called once the animation is called. This is where we will call the function that unmouts the screen

So unmounting the screen for components that use reanimated 2 will be handled in a callback to an animation. This is why I have to make sure we don't render the Animated.Code with the listener that unmounts the screen. I took the approach where we have a list of components that use reanimated 2, and for them we never set position. Based on position existance or non-existance, we then either render the Animated.Code or not.

Because the logic in this component is so complex, I wanted to avoid copying it. Especially that the reanimaed 1 logic will be removed at some point. So this code will gett a bit cleaner. Meanwhile maintaing two of such complicated components sounds like more trouble.

Test Plan

After refactoring nux tips overlay I renderd a nux tip. The animations work as expected, they don't get cancelled by the reaniamted 1 code.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/tooltip/nux-tips-overlay.react.js
94 ↗(On Diff #43359)

This is a hack - this doesn't preserve the behaviour for this component, but since we are not rendering it anywhere for now it doesn't matter.
This is so that we can slowly migrate this component to reaniamted 2 syntax over a couple of diffs

inka requested review of this revision.Aug 13 2024, 8:39 AM
tomek added inline comments.
native/media/video-playback-modal.react.js
484 ↗(On Diff #43359)

Is it really a tooltip?

native/navigation/overlay-context.js
10–11 ↗(On Diff #43359)

We should rename the variables to make them less confusing.

This revision is now accepted and ready to land.Aug 14 2024, 5:33 AM
native/navigation/overlay-navigator.react.js
68–73 ↗(On Diff #43359)

A bit confused about the new context type, but perhaps it's addressed in a later diff

I'm confused because I don't see a replacement for position. In order to deprecate position, don't we need to introduce a Reanimated 2 SharedValue here?

native/navigation/overlay-navigator.react.js
68–73 ↗(On Diff #43359)

Reanimated 2 is much different from reanimated 1. This is why, for example, there is no guide for migrating reanimated 1 to reanimated 2. When I asked about it I was told that basically these are two different libraries that are used so differently, it is practically not possible to create such guide.
Instead of using a position and interpolate, we will be now using entering and exiting animations. They don't need a SharedValue, but they need a wrapper component to be executed. This component is implemented in D13066.
animationCallback will be called after the exiting animation is finished.

Ah – the answer to my question is "Because we're using layout animations, rather than animations driven by a SharedValue." While this approach likely works for the NUX project, I suspect it won't work for the more complicated cases we need to support as part of ENG-8149. It's unfortunate that the approach you've chosen means that your work won't be generalizable.

Ah – the answer to my question is "Because we're using layout animations, rather than animations driven by a SharedValue." While this approach likely works for the NUX project, I suspect it won't work for the more complicated cases we need to support as part of ENG-8149. It's unfortunate that the approach you've chosen means that your work won't be generalizable.

To me, it seems like the chosen approach follows good practices. Entering and exiting animations should be handled like this. And for custom things, we're going to introduce a custom mechanism.