Details

Reviewers

atul
rohan
tomek

Commits

rCOMMdfa633540da4: [native] introduce reactions to inline sidebar

Summary

introduce reactions to inline sidebar. Right now, going with a temporary solution of just showing the number of likes right now, but in a subsequent diff will be extending this to show more information about the reaction message

Depends on D5878

Linear Task: ENG-2408

Test Plan

Please watch the demo video below to see how messages with reactions look like

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Dec 5 2022, 5:25 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 5 2022, 5:25 PM

Harbormaster completed remote builds in B13980: Diff 19153.Dec 5 2022, 5:35 PM

ginsu requested review of this revision.Dec 5 2022, 5:35 PM

ginsu edited the summary of this revision. (Show Details)Dec 5 2022, 5:37 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, rohan, tomek.

ginsu added a parent revision: D5812: [native] introduced reactions to chat item height measurer.

ginsu attached a referenced file: F272417: Screen Recording 2022-12-05 at 8.35.02 PM.mov. (Show Details)

ginsu edited the test plan for this revision. (Show Details)

add space between reaction and number

Harbormaster completed remote builds in B13983: Diff 19156.Dec 5 2022, 6:18 PM

ginsu added a child revision: D5815: [native] account for reactions in message item height methods.Dec 5 2022, 6:37 PM

ashoat added inline comments.Dec 6 2022, 6:39 AM

native/chat/composed-message.react.js
149 ↗	(On Diff #19156)	At some point we should rename this to `InlineEngagement` to better reflect that it includes reactions

tomek requested changes to this revision.Dec 6 2022, 7:01 AM

tomek added inline comments.

native/chat/composed-message.react.js
149 ↗	(On Diff #19156)	It seems like we had a component named like that e.g. D4115 but the diffs were abandoned for some reason.
native/chat/inline-sidebar.react.js
45 ↗	(On Diff #19156)	If additions and removals of reaction sum up to zero we shouldn't show them.
50 ↗	(On Diff #19156)	I don't think we can simply take a length when we allow removing a reaction.

This revision now requires changes to proceed.Dec 6 2022, 7:01 AM

tomek mentioned this in D5814: [web] introduce reactions to inline sidebar.Dec 6 2022, 7:05 AM

addressed feedback

Harbormaster completed remote builds in B14099: Diff 19299.Dec 12 2022, 8:49 PM

Could you share a screenshot with a case where multiple people liked a message?

native/chat/inline-sidebar.react.js
29 ↗	(On Diff #19299)	`$ReadOnlyMap`
51 ↗	(On Diff #19299)	For multiple reactions we would probably need to sort it in a way that is consistent between different messages. But we can worry about it later.
55–57 ↗	(On Diff #19299)	It looks ok for one reaction, but we don't separate multiple reactions with spaces.

address feedback

Harbormaster completed remote builds in B14134: Diff 19334.Dec 13 2022, 10:46 AM

fix small ui bug

In D5813#175174, @tomek wrote:

Could you share a screenshot with a case where multiple people liked a message?

ginsu attached a referenced file: F281900: Screen Recording 2022-12-12 at 11.50.26 PM.mov. (Show Details)Dec 13 2022, 11:14 AM

Harbormaster completed remote builds in B14136: Diff 19336.Dec 13 2022, 11:21 AM

Let's pull out the logic in reactionList to a helper function (probably in lib since I think this logic will also be needed on web?)

native/chat/inline-sidebar.react.js
50–64 ↗	(On Diff #19336)	Can we pull this logic out into a helper function? Presumably we're going to need the same function on `web`, can we move this to `lib`.