Page MenuHomePhabricator

[native] Convert LoggedOutModal to Reanimated 2 syntax
ClosedPublic

Authored by ashoat on May 24 2024, 1:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 5:28 PM
Unknown Object (File)
Mon, Dec 16, 7:06 PM
Unknown Object (File)
Mon, Dec 16, 7:06 PM
Unknown Object (File)
Mon, Dec 16, 7:06 PM
Unknown Object (File)
Mon, Dec 16, 7:06 PM
Unknown Object (File)
Mon, Dec 16, 7:06 PM
Unknown Object (File)
Mon, Dec 16, 7:06 PM
Unknown Object (File)
Mon, Dec 16, 6:19 PM
Subscribers

Details

Summary

Addresses ENG-8146.

Depends on D12207

Test Plan

I tested this diff stack by playing around with LoggedOutModal on my iOS simulator in my local environment:

  1. I navigated to LogInPanel and back
  2. I navigated to LegacyRegisterPanel and back
  3. I navigated to FullscreenSIWEPanel and back
  4. I navigated to QRCodeSignInNavigator and back
  5. I navigated to Registration and back

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

native/account/logged-out-modal.react.js
311–323 ↗(On Diff #40614)

These were just moved down (with useKeyboardHeight replaced by useRatchetingKeyboardHeight)

327–343 ↗(On Diff #40614)

These values are now determined in vanilla JavaScript by the getPanelPaddingTop worklet

350–353 ↗(On Diff #40614)

This block was just for initializing values. It arguably was never needed in the first place (we could have just initialized the values above)

354–364 ↗(On Diff #40614)

This logic is for handling when we set keyboardHeightValue to -1, which used to happen below in onPressLogIn and onPressRegister. The intention of that behavior was to wait for the keyboard to start appearing before starting the animation. The particular logic was there so that if the keyboard never appeared (eg. in an iOS simulator where the keyboard is disabled), then after the timeout clock ran out we would reset it to 0.

In this diff, I got rid of the logic that sets keyboardHeightValue to -1. In my experience playing around with the animations it didn't seem very important. Perhaps it mattered more before, or perhaps the new Reanimated 2 logic is snappier for some reason.

365–375 ↗(On Diff #40614)

This logic was for deciding when to initiate a new animation.

Reanimated 2 and 3 seem to handle this slightly differently. It looks like they allow you to set a shared value to a withTiming repeatedly, and then the C++ logic somehow intelligently decides whether a new animation needs to be initiated based on whether the target value has changed, and whether there is an animation in progress

376–379 ↗(On Diff #40614)

This was replaced by useRatchetingKeyboardHeight . I initially tried to use useAnimatedKeyboard from Reanimated directly, but found that the animation seems a bit slower than the keyboard. Here's a comparison:

useAnimatedKeyboarduseRatchetingKeyboardHeight
380–387 ↗(On Diff #40614)

This actually ran the animation, and is replaced by the withTiming call in the getPanelPaddingTop worklet

402–406 ↗(On Diff #40614)

This block was just for initializing values. It arguably was never needed in the first place (we could have just initialized the values above)

408–411 ↗(On Diff #40614)

This logic was for deciding when to initiate a new animation.

Reanimated 2 and 3 seem to handle this slightly differently. It looks like they allow you to set a shared value to a withTiming repeatedly, and then the C++ logic somehow intelligently decides whether a new animation needs to be initiated based on whether the target value has changed, and whether there is an animation in progress

412–418 ↗(On Diff #40614)

This actually ran the animation, and is replaced by the withTiming call in the getPanelOpacity worklet

420–423 ↗(On Diff #40614)

This is replaced by the completion callback (third parameter) passed to withTiming in getPanelOpacity. I could have alternately used useAnimatedReaction instead of the completion callback – can change to that if preferred

native/flow-typed/npm/react-native-reanimated_v2.x.x.js
548–554 ↗(On Diff #40614)

I added these types when I was initially trying to use useAnimatedKeyboard, but I decided against it (see video in inline comment above). Decided to keep the types for completeness's sake

native/keyboard/animated-keyboard.js
76–90 ↗(On Diff #40614)

This replaces ratchetAlongWithKeyboardHeight

native/account/logged-out-modal.react.js
465 ↗(On Diff #40614)

What is this replaced by?

native/flow-typed/npm/react-native-reanimated_v2.x.x.js
548 ↗(On Diff #40614)

In docs this is called AnimatedKeyboardInfo, can we call it that as well?

550 ↗(On Diff #40614)

The docs say that state can have four values. Why do we have five?

native/account/logged-out-modal.react.js
465 ↗(On Diff #40614)

It's actually not replaced. I'm not sure it needs to be, given that the Keyboard.dismiss() call will result in it eventually being set to 0... the animation is clean without it, and having this here breaks the clean separation introduced in D12207

native/flow-typed/npm/react-native-reanimated_v2.x.x.js
548 ↗(On Diff #40614)

Sure, I'll rename it

550 ↗(On Diff #40614)

The docs for version 3 have an additional section where they spell out the types explicitly, and an UNKNOWN is mentioned there that isn't covered in the "Possible values" section you reference. It's possible that UNKNOWN is only there for version 3, but I suspect it's there for version 2 as well, and I'm sure we gain anything from excluding it given we plan to upgrade to Reanimated 3 soon. Nevertheless, I can go ahead and check the actual source to confirm this

Rename AnimatedKeyboardObject to AnimatedKeyboardInfo in libdef

This revision is now accepted and ready to land.May 27 2024, 1:15 AM