Page MenuHomePhabricator

[native] Introduce `inlineSidebar` animation when opening message tooltip to avoid visual issues.
ClosedPublic

Authored by jacek on Aug 12 2022, 2:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 9:13 AM
Unknown Object (File)
Sat, Nov 9, 9:13 PM
Unknown Object (File)
Sat, Nov 9, 9:12 PM
Unknown Object (File)
Sat, Nov 9, 6:36 PM
Unknown Object (File)
Sat, Nov 9, 6:30 PM
Unknown Object (File)
Sat, Nov 9, 3:46 PM
Unknown Object (File)
Oct 23 2024, 6:15 PM
Unknown Object (File)
Oct 23 2024, 1:49 PM
Subscribers

Details

Summary

Fix visual issues when opening message tooltip. For the animation, it requires rendering the second InlineSidebar component on the top, so it could diffuse the message.

Before:

After:

Test Plan

Tested the animation on both platforms for multimedia and text messages. Confirmed if "go to sidebar" animation is not affected.

Diff Detail

Repository
rCOMM Comm
Branch
jacek/inline-sidebar-3
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

jacek held this revision as a draft.
abosh added 2 blocking reviewer(s): atul, tomek.

Tested this locally and it looks great! The visual change of the tooltip 'fading' through the message when holding down on the message is nice.

Screen Shot 2022-08-12 at 12.30.04 PM.png (2×1 px, 1 MB)

Once again adding @tomek and @atul as blocking.

Edit, also looks as expected on Android (there's no emojis on the robotext message since I forgot to supply a reactions prop there):

image.png (2×1 px, 1 MB)

ashoat requested changes to this revision.Aug 12 2022, 10:20 AM

Based on @abosh's screenshots, it looks like this diff is attempting to land fake placeholder emojis on master...

This revision now requires changes to proceed.Aug 12 2022, 10:21 AM

Based on @abosh's screenshots, it looks like this diff is attempting to land fake placeholder emojis on master...

I was just playing around with the code, there are no placeholder emojis (I had to supply the reactions prop, etc.)

This revision now requires review to proceed.Aug 12 2022, 11:02 AM

Looks great! Tested it out on physical device and things are smooth.

Defer to @ashoat on reviewing the Reanimated stuff, but looks like he's already accepted.

native/chat/inline-sidebar.react.js
17–25 ↗(On Diff #15599)

Can we clean this up a bit by clumping all of these constants into some sort of object so it's a little cleaner?

165–183 ↗(On Diff #15599)

Personal preference is to use if statements instead of switch statements, but don't feel strongly at all... whatever you prefer.

Looks like there are two other instances of switch keyword (getRelationshipActionText and daysInMonth) in the JS codebase so I'm guessing it's fine.

ashoat requested changes to this revision.Aug 15 2022, 11:13 AM
  1. I should've reassigned instead of accepting... my intention was just to undo my "Request Changes" from earlier
  2. @atul – Reanimated stuff is not listed in my purview here, so it's something we'll need reviewers like you and @tomek to ramp up on

I don't have time to get up to speed on all of this, so ideally somebody else on the team can take that on. If not I'll need some additional context from @jacek to review

native/chat/inline-sidebar.react.js
165–183 ↗(On Diff #15599)

I don't like switch-case either... it increases indentation, isn't consistent with other C-syntax, and has behavior that can feel unexpected (cases passing into other cases)

187 ↗(On Diff #15599)
  1. A little confused about the point of multiplying something by zero
  2. Do we have any need to animate based on isOpeningSidebar? What does isOpeningSidebar track, and why does it need to exist?
This revision now requires changes to proceed.Aug 15 2022, 11:13 AM
native/chat/inline-sidebar.react.js
187 ↗(On Diff #15599)

I discussed this solution with @tomek, and we concluded it will be the simplest way to achieve correct animation.
The reason here is to prevent the component from being displayed after we press "open sidebar" what causes screen transition and additional message animations.
Right now, we have some complex logic related to progress variable and at the end of opening sidebar animation the component would appear again. As isOpeningSidebar variable is true after screen transition starts, we use it to prevent displaying the inline sidebar while sidebar animation.

Still confused. Sorry, I'm not a great reviewer for things like this... since I don't have much time per review, I sometimes need to be "spoonfed" the explanation more so than other reviewers

native/chat/inline-sidebar.react.js
187 ↗(On Diff #15599)
  1. We're hiding InlineSidebar after it's pressed? Why is that? Can you share a video of the whole animation of opening a sidebar?
  2. If we're multiplying by 0, why not just set opacity: 0?
native/chat/inline-sidebar.react.js
187 ↗(On Diff #15599)
  1. We hide InlineSidebar after we press the message to display the tooltip - so the message could "fade" through it. Then we don't want to display it again when we move to the sidebar. The whole animation is already in diff description.

If we don't set opacity to 0 after we open the sidebar, the effect would be (and it's what we want to avoid):

  1. I think, the same effect could be achieved with the following code, which is probably much cleaner:
const opacity = isOpeningSidebar
  ? 0
  : interpolateNode(progress, {
      inputRange: [0, 1],
      outputRange: [1, 0],
      extrapolate: Extrapolate.CLAMP,
    });

Address Atul's and Ashoat's comments

I get it now!! Thanks for the explanation

tomek added inline comments.
native/chat/inline-sidebar.react.js
165–183 ↗(On Diff #15599)

My personal preference is to use switch in case like this. This syntax will become more widely used as we use more Rust where match is similar and really beneficial. But obviously, if switch doesn't feel readable for the team we can keep avoiding it.

This revision is now accepted and ready to land.Aug 19 2022, 8:01 AM