Page MenuHomePhabricator

[web] Display a pin icon next to pinned messages in chat
ClosedPublic

Authored by rohan on Apr 18 2023, 1:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 7:11 AM
Unknown Object (File)
Sun, Nov 24, 6:41 AM
Unknown Object (File)
Sun, Nov 24, 6:41 AM
Unknown Object (File)
Sun, Nov 24, 6:09 AM
Unknown Object (File)
Sun, Nov 24, 5:53 AM
Unknown Object (File)
Sun, Nov 24, 5:26 AM
Unknown Object (File)
Tue, Nov 12, 12:33 PM
Unknown Object (File)
Tue, Nov 12, 12:29 PM
Subscribers

Details

Summary

Next to each pinned message, we want to display a pin icon in the color of the thread. To do this, one main change that was made is we need to determine whether the ComposedMessage (Message -> TextMessage or MultimediaMessage -> ComposedMessage) is rendered within a modal, or just in chat. This is because we consider two scenarios:

  1. The message is displayed in the toggle pin modal or the thread pinned messages modal - here, we want to avoid displaying the pin icon next to messages
  1. The message is rendered in the chat view, and there we want to display the pin icon next to messages.

As opposed to using a Context, I figured that that propagating the props through the three components in the rendering hierarchy would be the most efficient, so that's what this diff does.

Linear: https://linear.app/comm/issue/ENG-3455/show-a-pin-icon-next-to-pinned-messages

Depends on D7382

Test Plan

Test video will be attached to demonstrate that the icon is not shown in modals, and the icon is correctly positioned / colored.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 18 2023, 2:09 PM
Harbormaster failed remote builds in B18482: Diff 25313!
rohan requested review of this revision.Apr 18 2023, 2:11 PM
tomek added inline comments.
web/chat/chat-message-list.css
245–251 ↗(On Diff #25313)

Have you checked how it looks like on different window resolutions?

web/chat/composed-message.react.js
73 ↗(On Diff #25313)

Being so concrete in prop name is bad for maintainability:

  • it doesn't show to a caller what is affected by this prop
  • if we decide to change pin appearance in a new context, it will be hard to discover that we already have something like that

Maybe rename it to e.g. allowDisplayingPinIndicator, or something similar?

168 ↗(On Diff #25313)

Just a nit, but it's better to base this on position

This revision is now accepted and ready to land.Apr 19 2023, 2:38 AM
web/chat/chat-message-list.css
245–251 ↗(On Diff #25313)

Yeah, as I changed my window size the pin icon looked good and stayed positioned in the right place

Replace shouldDisplayPinIndicator and prop drilling by just setting
isPinned to false for the modifiedItem displayed in the modals

Revert back to shouldDisplayPinIndicator as the new approach caused
problems in the modals