Details

Reviewers

atul
tomek
ashoat

Commits

rCOMM2d9f99f71578: [web/lib] introduce MessageReactionsModal

Summary

introduce MessageReactionsModal. MessageReactionsModal shows all the users who have reacted to a message, what their username is and what reaction they used. I decided to punt the implemenation of the filter buttons at the top of the design since every message will be a like for the moment

Depends on D6301
Linear Task: ENG-2718
Figma

Test Plan

Please watch the demo to see how MessageReactionsModal works

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Jan 18 2023, 1:04 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 18 2023, 1:04 PM

ginsu edited the summary of this revision. (Show Details)Jan 18 2023, 1:07 PM

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

ginsu added reviewers: atul, tomek.

ginsu added a parent revision: D6301: [lib] add relative user info to users in messageReactionInfo.

ginsu attached a referenced file: F334090: Screen Recording 2023-01-18 at 3.55.20 PM.mov. (Show Details)

Harbormaster completed remote builds in B15443: Diff 21054.Jan 18 2023, 1:20 PM

ginsu requested review of this revision.Jan 18 2023, 1:20 PM

tomek requested changes to this revision.Jan 19 2023, 3:19 AM

tomek added inline comments.

web/modals/chat/message-reactions-modal.css
8 ↗	(On Diff #21054)
web/modals/chat/message-reactions-modal.react.js
19 ↗	(On Diff #21054)	This name is misleading. The value is not only a collection of users because it also contains a reaction. Also, a user can appear multiple times in it.
22–24 ↗	(On Diff #21054)	Maybe it is better to iterate over `entries` so that we don't need to use an invariant?
26 ↗	(On Diff #21054)	It isn't a good idea to just map the values without sorting. It seems like a new reaction might result in all the users being reshuffled. Can we avoid this?
29–36 ↗	(On Diff #21054)

This revision now requires changes to proceed.Jan 19 2023, 3:19 AM

address tomek's feedback

ginsu added inline comments.Jan 19 2023, 10:06 AM

web/modals/chat/message-reactions-modal.react.js
26 ↗	(On Diff #21054)	This is a good point. I wrote a small sorting algorithm to address your comment, but having a user who reacted to the same message with 2 or more reactions and how we display that in this modal is probably something we want to rethink in the UI design

Harbormaster completed remote builds in B15479: Diff 21101.Jan 19 2023, 10:15 AM

ashoat added inline comments.Jan 19 2023, 2:08 PM

web/modals/chat/message-reactions-modal.react.js
25–32 ↗	(On Diff #21101)

address ashoat's comments

Harbormaster completed remote builds in B15487: Diff 21127.Jan 19 2023, 8:59 PM

tomek requested changes to this revision.Jan 20 2023, 10:09 AM

tomek added inline comments.

web/modals/chat/message-reactions-modal.react.js
26 ↗	(On Diff #21054)	Yeah, definitely. I'd prefer having the entries first sorted by reaction and then by user. Even if we decide to keep the current approach, we still should sort by the notification as the second step. Also, I think it would be more intuitive for users if we were sorting by usernames (`stringForUserExplicit`?) instead of the ids.

This revision now requires changes to proceed.Jan 20 2023, 10:09 AM

address tomek's feedback

Harbormaster completed remote builds in B15544: Diff 21205.Jan 23 2023, 9:48 AM

ashoat added inline comments.Jan 23 2023, 10:00 AM

web/modals/chat/message-reactions-modal.react.js
32 ↗	(On Diff #21205)	This seems pretty complicated, can we simplify with Lodash `_sortBy`? We could pass it two predicates, one for number of reactions and one for username