Page MenuHomePhabricator

[web/lib] introduce MessageReactionsModal
ClosedPublic

Authored by ginsu on Jan 18 2023, 1:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 4:35 PM
Unknown Object (File)
Thu, Dec 19, 4:11 PM
Unknown Object (File)
Thu, Dec 19, 2:20 PM
Unknown Object (File)
Wed, Dec 18, 5:29 PM
Unknown Object (File)
Wed, Dec 18, 4:06 PM
Unknown Object (File)
Wed, Dec 18, 4:05 PM
Unknown Object (File)
Wed, Dec 18, 2:11 PM
Unknown Object (File)
Nov 11 2024, 11:55 PM
Subscribers

Details

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 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
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

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

address ashoat's comments

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
web/modals/chat/message-reactions-modal.react.js
32

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

use lodash _sortBy method

ashoat added inline comments.
web/modals/chat/message-reactions-modal.react.js
42 ↗(On Diff #21227)

To reduce indentation and improve readability, I would assign this function to a variable named eg. sortByNumReactions

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

address ashoat's comments

ginsu retitled this revision from [web] introduce MessageReactionsModal to [web/lib] introduce MessageReactionsModal.Jan 24 2023, 8:44 AM
This revision was automatically updated to reflect the committed changes.