Page MenuHomePhabricator

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

Authored by rohan on Apr 11 2023, 5:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 4:51 AM
Unknown Object (File)
Sat, Nov 2, 12:27 PM
Unknown Object (File)
Sat, Nov 2, 12:27 PM
Unknown Object (File)
Mon, Oct 28, 7:43 AM
Unknown Object (File)
Sat, Oct 19, 10:27 PM
Unknown Object (File)
Oct 9 2024, 11:50 AM
Unknown Object (File)
Sep 28 2024, 9:32 AM
Unknown Object (File)
Sep 28 2024, 9:32 AM
Subscribers

Details

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

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!
rohan requested review of this revision.Apr 11 2023, 6:02 AM

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

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

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

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

Rebase for passing tests and address feedback to use dispatchActionPromise

Replace invariant that is not needed

Rename ThreadPinnedMessagesModal -> MessageResultsModal

General refactoring to make the modal more reusable (first run-through)