Page MenuHomePhabricator

[native] changed tooltip animation to slide up/down on enter and exit
ClosedPublic

Authored by ginsu on Nov 8 2022, 8:24 AM.
Tags
None
Referenced Files
F3389504: D5557.diff
Fri, Nov 29, 7:24 PM
Unknown Object (File)
Thu, Nov 28, 4:54 AM
Unknown Object (File)
Tue, Nov 26, 12:23 PM
Unknown Object (File)
Sun, Nov 24, 11:50 AM
Unknown Object (File)
Sun, Nov 24, 11:50 AM
Unknown Object (File)
Sun, Nov 24, 11:50 AM
Unknown Object (File)
Sun, Nov 24, 11:50 AM
Unknown Object (File)
Sun, Nov 24, 11:50 AM

Details

Summary

Using layout animations the fixed tooltip now slides in and out from the bottom of the screen instead of from the side. Here is the discussion about animation for further context.

Reanimated layout transitions docs


Depends on D5556

Linear Task: ENG-2037

Test Plan

EDIT: Updated the demo videos to show more edge cases

Please watch the demos to see the changes I made:

Before:

After:
iOS:

Android:

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu requested review of this revision.Nov 8 2022, 8:38 AM
native/navigation/tooltip.react.js
489 ↗(On Diff #18212)

Slight delay is needed, otherwise, the animation will happen as navigation is also happening and it won't render the animation correctly

522–529 ↗(On Diff #18212)

Timeout is to give enough time for the animation to complete before the app navigates away from this screen

native/types/styles.js
36–40 ↗(On Diff #18212)

taken from renanimated AnimatedComponentProps

atul added a reviewer: ashoat.

Looks reasonable, adding @ashoat as blocking since I don't really understand reanimated

native/navigation/tooltip.react.js
489 ↗(On Diff #18212)

These are pretty significant differences, can you explain your reasoning here? It it based on what you observed on Simulator/Emulator or physical devices or something else?

native/navigation/tooltip.react.js
489 ↗(On Diff #18212)

I tested on my iPhone 11 and the Google Pixel 6a, and these were the smallest times when the animation was consistently working well

ashoat requested changes to this revision.Nov 8 2022, 11:39 AM
ashoat added inline comments.
native/navigation/tooltip.react.js
502–504 ↗(On Diff #18212)

Can we avoid all the computation above for setting tooltip if hideTooltip is true? Could this condition be the start of a large if-else-if-else-if-else block? Does itemsStyle matter if hideTooltip is true?

521–530 ↗(On Diff #18212)

I think this would look cleaner this way

522–529 ↗(On Diff #18212)

This is messy... is there no better way to do this? Eg. here's how we handle cleanup in a Reanimated 1 world: https://github.com/CommE2E/comm/blob/master/native/navigation/overlay-navigator.react.js#L303-L305

This revision now requires changes to proceed.Nov 8 2022, 11:39 AM
native/navigation/tooltip.react.js
521–530 ↗(On Diff #18212)

Seems like JS does not have a builtin sleep function. I can still create our own sleep function or would this be a cleaner approach?

onPressBackdrop = () => {
  const navigateBackCallback = () => this.props.navigation.goBackOnce();

  if (this.location !== 'fixed') {
    navigateBackCallback();
    return;
  }

  this.props.setHideTooltip(true);
  setTimeout(navigateBackCallback, 100);
};
native/navigation/tooltip.react.js
521–530 ↗(On Diff #18212)

git grep sleep, but note my other concerns... ideally we don't need this at all

native/navigation/tooltip.react.js
521–530 ↗(On Diff #18212)

Also just came across this for layout animations, I think this is actually what we want

native/navigation/tooltip.react.js
521–530 ↗(On Diff #18212)

Don't think we can just call our normal code from a worklet, but perhaps there's some special way to do it

addressed ashoat's comments

ashoat requested changes to this revision.Nov 9 2022, 8:20 AM

Awesome!! Really close, just want to avoid redefining some things on each render if possible

native/navigation/tooltip.react.js
495 ↗(On Diff #18273)

This can also be defined in the root context

497–498 ↗(On Diff #18273)

This can be defined in the root context

500–504 ↗(On Diff #18273)

Define this on the class, no need to redefine it every time

505–510 ↗(On Diff #18273)

Not sure if a worklet can be defined on the class or not, but if it can we should do it

540–544 ↗(On Diff #18273)

I would go back to this now that the code is simplified

This revision now requires changes to proceed.Nov 9 2022, 8:20 AM
native/navigation/tooltip.react.js
497–498 ↗(On Diff #18273)

@ashoat just to clairfy, when you mean root context you mean outside the if/else block correct?

native/navigation/tooltip.react.js
497–498 ↗(On Diff #18273)

Outside all blocks

native/navigation/tooltip.react.js
500–504 ↗(On Diff #18273)

I don't know if I am missing something here, but I don't think worklets or the callback that runOnJS calls can be defined in the class. When I define goBackCallback on the class and call it through runOnJS(this.goBackCallback)() I keep getting error...

Screen Shot 2022-11-09 at 12.31.53 PM.png (1×1 px, 1 MB)

Will keep digging around, but wondering if there is an alternative approach that would be just as good

native/navigation/tooltip.react.js
500–504 ↗(On Diff #18273)

I wonder if this has anything to do with why I can't call a function defined in the class

trying new strategy where the worklet is defined in the connected component and passed down as prop

need to fix one edge case

switched actionSheetShown from state value to reanimated shared value

This revision is now accepted and ready to land.Nov 14 2022, 5:30 PM