Details

Reviewers

atul
ginsu
tomek
inka

Commits

rCOMM6bda99d0eafd: [web] Display the pinned messages for the thread when the banner is clicked

Summary

The pinned messages modal for the thread should display when the user clicks on the banner indicating the number of pinned messages for that given thread. Each message should display the inline engagement (reactions and replies), based on the designs. This is different from the message being displayed in the toggle pin modal, as the designs there indicated that we wanted to remove the inline engagement.

Depends on D7310

Linear: https://linear.app/comm/issue/ENG-3449/display-the-pinned-messages-for-the-thread-when-the-banner-is-clicked

There is a follow-up task to help refactor usability for the messages search screen here: https://linear.app/comm/issue/ENG-3644/refactor-file-names-details-to-make-the-thread-pin-modal-reusable-by

Test Plan

Pin several messages, with / without inline engagement and a mix between text and media. Confirm that the media gallery looks as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Apr 11 2023, 5:47 AM

Herald added a subscriber: ashoat. · View Herald TranscriptApr 11 2023, 5:47 AM

rohan edited the summary of this revision. (Show Details)Apr 11 2023, 5:48 AM

rohan added a parent revision: D7310: [web] Display the number of pinned messages in a banner across chat.

rohan edited the summary of this revision. (Show Details)

rohan edited the test plan for this revision. (Show Details)Apr 11 2023, 5:57 AM

rohan attached a referenced file: F478961: Screen Recording 2023-04-11 at 8.57.23 AM.mov. (Show Details)

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 11 2023, 6:02 AM

Harbormaster failed remote builds in B18228: Diff 24964!

Not blocking review while I figure out a failed test in D7309

rohan added a child revision: D7382: [web] Allow interaction with inline engagement from the thread pinned messages modal.Apr 11 2023, 6:09 AM

rohan edited the summary of this revision. (Show Details)Apr 11 2023, 2:48 PM

rohan added a reviewer: inka.

tomek requested changes to this revision.Apr 12 2023, 5:34 AM

tomek added inline comments.

web/chat/thread-top-bar.react.js
40–42 ↗	(On Diff #24964)	As noted in D7309 - it would be great if we could avoid this.
web/modals/chat/thread-pinned-messages-modal.css
2–6 ↗	(On Diff #24964)	I think it is more common to use border instead of background to render a line
6 ↗	(On Diff #24964)	Can we use a const?
web/modals/chat/thread-pinned-messages-modal.react.js
33 ↗	(On Diff #24964)	Why do we need this invariant?
34–35 ↗	(On Diff #24964)	A similar code can be found in D7310 - can we avoid duplicating it? Also `singleOrPlural` isn't a great name... maybe something like `pluralizedMessage` or `messageNoun` might work better?
39 ↗	(On Diff #24964)	Can we use `dispatchActionPromise` for this? Without this it will be hard to e.g. show a loading state.
44 ↗	(On Diff #24964)	What `translated` means in this context?
47–50 ↗	(On Diff #24964)	You can use a shorthand
66 ↗	(On Diff #24964)	Why do we need to check this?
74–82 ↗	(On Diff #24964)	This might be really inefficient - for all the messages we iterate through every message `O(n^2)`. We can e.g. create an object/map which contains id => messageInfo mappings and could be used instead of `findIndex` call.
88–94 ↗	(On Diff #24964)	This might be even `O(n^2logn)` - can we create a mapping from id to an index and use it instead of `findIndex`?
100–123 ↗	(On Diff #24964)	Use a shorthand
108–120 ↗	(On Diff #24964)	This looks really similar to D7309 - can we reuse?
125–135 ↗	(On Diff #24964)

This revision now requires changes to proceed.Apr 12 2023, 5:34 AM

By the way - on the local environment fetching messages is fast. On the prod it might be a lot slower - should we display a spinner or something? How would the UI look if messages are being fetched?

Also, do we plan to fetch all the pinned messages at once or fetch them incrementally while the user scrolls? I guess incremental approach would be nice, but might increase the complexity and it's not too likely to have a lot of pinned messages.

On the prod it might be a lot slower - should we display a spinner or something? How would the UI look if messages are being fetched?

Should be covered in designs – I think the default behavior is to show a spinner at the bottom of the list, so if there is nothing fetched yet that spinner should be the only thing in the list. Worth checking with Ted though

rohan added inline comments.Apr 12 2023, 11:39 PM

web/chat/thread-top-bar.react.js
40–42 ↗	(On Diff #24964)	Responded to your comment in D7309!
web/modals/chat/thread-pinned-messages-modal.react.js
34–35 ↗	(On Diff #24964)	Yeah, I agree that we can avoid duplicating it. In terms of making this code more reusable for the message search modal, I'm removing the hard coded modalName from the component in the revision, since the modal name should be different for the search modal.
44 ↗	(On Diff #24964)	I used `translated` for 'converted' or 'changed', since we created a `MessageInfo` from a RawMessageInfo`, but happy to rename if needed
108–120 ↗	(On Diff #24964)	It's similar but not identical since here we want to render the inline engagement, so we maintain the properties that were defaulted to `false` and `{}` in D7309. I can look into seeing if I can somehow unify this creation though to reduce repeated logic

ESLint + Address Feedback

In D7380#220210, @tomek wrote:

By the way - on the local environment fetching messages is fast. On the prod it might be a lot slower - should we display a spinner or something? How would the UI look if messages are being fetched?

Yeah, I think the behavior should follow similar to how the loader displays when fetching pinned messages

Also, do we plan to fetch all the pinned messages at once or fetch them incrementally while the user scrolls? I guess incremental approach would be nice, but might increase the complexity and it's not too likely to have a lot of pinned messages.

At the moment there's no pagination in the query, but there's a task for this: https://linear.app/comm/issue/ENG-3397/add-pagination-to-fetching-message-pins-for-a-thread

web/modals/chat/thread-pinned-messages-modal.react.js
39 ↗	(On Diff #24964)	Sure makes sense, I'll start working on that locally

Harbormaster failed remote builds in B18297: Diff 25075!Apr 12 2023, 11:57 PM

tomek accepted this revision.Apr 13 2023, 3:32 AM

tomek added inline comments.

web/modals/chat/thread-pinned-messages-modal.react.js
68–73 ↗	(On Diff #25075)	This is a strange pattern - a result of `map` should be used somehow and not ignored. Also, a function used in `map` should be pure. If we're calling this just for its side effects, we can use e.g. `forEach`. If we want to keep the `map`, we can create an array of key-value pairs and use it as `Map`'s constructor argument.
75–77 ↗	(On Diff #25075)	This comment is slightly misleading - we're not calling `sort` here
39 ↗	(On Diff #24964)	Are you going to update this diff with it or create a new one? This change should be simple and updating this one probably makes more sense.

This revision is now accepted and ready to land.Apr 13 2023, 3:32 AM

rohan added inline comments.Apr 14 2023, 11:10 AM

web/modals/chat/thread-pinned-messages-modal.react.js
68–73 ↗	(On Diff #25075)	Makes sense, I'll consider which one makes the most sense and go with that
75–77 ↗	(On Diff #25075)	Thanks for catching that, I forgot to update that comment
39 ↗	(On Diff #24964)	Yeah I'm going to update this diff