Page MenuHomePhabricator

[native] introduce reaction selection popover component
ClosedPublic

Authored by ginsu on Jan 29 2023, 9:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 5:21 PM
Unknown Object (File)
Thu, Dec 19, 2:54 PM
Unknown Object (File)
Thu, Dec 19, 2:40 PM
Unknown Object (File)
Thu, Dec 19, 10:45 AM
Unknown Object (File)
Nov 15 2024, 9:28 PM
Unknown Object (File)
Nov 8 2024, 3:10 AM
Unknown Object (File)
Nov 8 2024, 3:10 AM
Unknown Object (File)
Nov 8 2024, 3:09 AM
Subscribers

Details

Summary

introduce the ReactionSelectionPopover component. For the default emojis, I could not fit all the emojis mentioned in the linear task in the popover. So I went with FB messenger's default emojis for now, but we can always change it in the future. Also will be cleaning up the animation in a subsequent diff

Facebook Messengers default emojis:

Screenshot 2023-01-30 at 12.24.50 AM.png (148×608 px, 63 KB)

Figma


Depends on D6487
Linear Task: ENG-2779

Test Plan

Please watch the demo video to see what the reaction selection popover component looks like

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu requested review of this revision.Jan 29 2023, 9:31 PM
tomek added inline comments.
native/chat/reaction-selection-popover.react.js
34 ↗(On Diff #21548)

Wondering if we really want to close it here. We can also consider keeping it open when someone wants to react multiple times. Not sure which is better.

56 ↗(On Diff #21548)

Are we animating something here?

native/navigation/tooltip.react.js
621 ↗(On Diff #21548)

Why do we render this based on tooltipLocation value?

This revision is now accepted and ready to land.Jan 30 2023, 5:22 AM

move reaction selection popover from tooltip to text/multimedia/robotext message tooltip buttons

ginsu edited the test plan for this revision. (Show Details)
ginsu added a reviewer: atul.
ginsu edited the summary of this revision. (Show Details)
ginsu requested review of this revision.Jan 31 2023, 1:41 PM

rerequesting review because made a significant updates to the diff

The approach is ok, but it isn't a good practice to copy the code. For all the messages we use (almost?) the same logic - can we reuse it?

This revision is now accepted and ready to land.Feb 1 2023, 2:59 AM
In D6454#194196, @tomek wrote:

The approach is ok, but it isn't a good practice to copy the code. For all the messages we use (almost?) the same logic - can we reuse it?

Completely agree, implementing a wrapper component for the three message tooltip components could be a good idea. I will create a task and link it to this diff later today/before I land

rebase for changes needed in D6455

The approach is ok, but it isn't a good practice to copy the code. For all the messages we use (almost?) the same logic - can we reuse it?

Created a task to track