Page MenuHomePhabricator
Differential D6454

[native] introduce reaction selection popover component
ClosedPublic
Actions

Authored by ginsu on Jan 29 2023, 9:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 3:10 AM
Unknown Object (File)
Fri, Nov 8, 3:10 AM
Unknown Object (File)
Fri, Nov 8, 3:09 AM
Unknown Object (File)
Sat, Nov 2, 5:17 PM
Unknown Object (File)
Fri, Nov 1, 11:37 AM
Unknown Object (File)
Fri, Nov 1, 11:33 AM
Unknown Object (File)
Fri, Nov 1, 11:33 AM
Unknown Object (File)
Fri, Nov 1, 11:33 AM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
tomek
atul
Commits
rCOMM9e594b75db07: [native] introduce reaction selection popover component
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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu created this revision.Jan 29 2023, 9:16 PM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 29 2023, 9:16 PM
ginsu edited the summary of this revision. (Show Details)Jan 29 2023, 9:21 PM
ginsu edited the summary of this revision. (Show Details)Jan 29 2023, 9:25 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added a parent revision: D6453: [native] introduce useReactionSelectionPopoverPosition hook.
ginsu attached a referenced file: F350873: Screen Recording 2023-01-30 at 12.22.48 AM.mov. (Show Details)
ginsu attached a referenced file: F350877: Screenshot 2023-01-30 at 12.24.50 AM.png. (Show Details)
Harbormaster completed remote builds in B15802: Diff 21548.Jan 29 2023, 9:31 PM
ginsu requested review of this revision.Jan 29 2023, 9:31 PM
ginsu added a child revision: D6455: [native] implement emoji keyboard button on reaction selection popover.Jan 29 2023, 9:36 PM
tomek accepted this revision.Jan 30 2023, 5:22 AM
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
ginsu updated this revision to Diff 21712.Jan 31 2023, 1:36 PM

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

ginsu edited the summary of this revision. (Show Details)Jan 31 2023, 1:38 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added a reviewer: atul.
ginsu attached a referenced file: F354289: Screen Recording 2023-01-31 at 4.31.34 PM.mov. (Show Details)
ginsu edited the summary of this revision. (Show Details)
ginsu added a parent revision: D6487: [native] introduce useSendReaction hook.
ginsu requested review of this revision.Jan 31 2023, 1:41 PM

rerequesting review because made a significant updates to the diff

ginsu removed a parent revision: D6453: [native] introduce useReactionSelectionPopoverPosition hook.Jan 31 2023, 1:41 PM
Harbormaster completed remote builds in B15906: Diff 21712.Jan 31 2023, 2:00 PM
tomek accepted this revision.Feb 1 2023, 2:59 AM

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
ginsu added a comment.Feb 1 2023, 9:15 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

ginsu updated this revision to Diff 21791.Feb 1 2023, 1:22 PM

rebase for changes needed in D6455

ginsu updated this revision to Diff 21797.Feb 1 2023, 1:42 PM

rebase before landing

Harbormaster completed remote builds in B15956: Diff 21791.Feb 1 2023, 1:43 PM
Harbormaster completed remote builds in B15961: Diff 21797.Feb 1 2023, 2:02 PM
ginsu added a comment.Feb 1 2023, 2:04 PM

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

Closed by commit rCOMM9e594b75db07: [native] introduce reaction selection popover component (authored by ginsu). · Explain WhyFeb 1 2023, 2:32 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM9e594b75db07: [native] introduce reaction selection popover component.

Revision Contents
Changeset List

Diff 21811

View Options

native/chat/multimedia-message-tooltip-button.react.js

Loading...
View Options

native/chat/reaction-selection-popover.react.js

Loading...
View Options

native/chat/robotext-message-tooltip-button.react.js

Loading...
View Options

native/chat/text-message-tooltip-button.react.js

Loading...
View Options

native/navigation/tooltip.react.js

Loading...
View Options

native/themes/colors.js

Loading...