Details

Reviewers

atul
ginsu
tomek

Commits

rCOMMb1858ff3fc16: [web] Pop up a pin/unpin modal when the tooltip icon is clicked

Summary

Following the designs, we need an intermediate modal when a user tries to pin / unpin a message that 1) displays the message and 2) allows the user to confirm their requested action.

Because we are rendering a Message, there were two Providers we needed to avoid the invariants.

Firstly, the useMessageTooltipReplyAction requires an inputState. I define the inputState in tooltip-utils since the target message should
have access to the context, as opposed to it being null in the modal.

Secondly, the messageListContext is required to be defined since Message renders a TextMessage.

Linear: https://linear.app/comm/issue/ENG-3446/pop-up-a-pinunpin-modal-when-the-tooltip-icon-is-clicked

Depends on D7308

Test Plan

The video is linked below, I sync'd offline with Ted to confirm the designs

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Apr 4 2023, 7:20 PM

Herald added a subscriber: ashoat. · View Herald TranscriptApr 4 2023, 7:20 PM

rohan edited the summary of this revision. (Show Details)Apr 4 2023, 7:21 PM

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

rohan attached a referenced file: F463410: Screen Recording 2023-04-04 at 10.06.15 PM.mov. (Show Details)

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

rohan added inline comments.Apr 4 2023, 7:29 PM

web/modals/chat/toggle-pin-modal.react.js
59–64 ↗	(On Diff #24649)	As explained in the inline comment, we want viewer to be `false` so the message is left-aligned and the username is displayed (feedback from Ted design wise)
65–70 ↗	(On Diff #24649)	This is because we don't want to 'double render' the username and date for messages that will already have them rendered based on the conditions in the `ComposedMessage` component and the `Message` component

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 4 2023, 7:34 PM

Harbormaster failed remote builds in B17993: Diff 24649!

rohan added a child revision: D7310: [web] Display the number of pinned messages in a banner across chat.Apr 4 2023, 7:35 PM

rohan mentioned this in D7308: [lib/web] Add pin/unpin icon in the tooltip.Apr 5 2023, 5:27 AM

Fix eslint warnings

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 5 2023, 5:50 AM

Harbormaster failed remote builds in B17996: Diff 24654!

Update designs

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

rohan added inline comments.Apr 11 2023, 5:09 AM

web/components/pinned-message.react.js
31–34	This is because we don't want to 'double render' the username, since we're rendering `Message`, which renders `ComposedMessage`, the component sometimes displays the username inherently based on some conditions.
web/modals/chat/toggle-pin-modal.react.js
94	I separated this into it's own component because we need to re-use `PinnedMessage` when the user wants to view a list of all pinned messages for the thread in a separate modal.

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

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

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

Harbormaster failed remote builds in B18222: Diff 24958!

I still need to figure out what is causing tooltip-utils.test.js to fail, but after attempting to debug locally, I'd prefer to unblock review and continuing figuring it out locally

rohan mentioned this in D7310: [web] Display the number of pinned messages in a banner across chat.Apr 11 2023, 5:30 AM

rohan mentioned this in D7380: [web] Display the pinned messages for the thread when the banner is clicked.Apr 11 2023, 6:02 AM

rohan mentioned this in D7382: [web] Allow interaction with inline engagement from the thread pinned messages modal.Apr 11 2023, 8:26 AM

This diff causes eslint to fail. Please fix it.

web/components/pinned-message.css
6	You can use a shorthand
20	Do we have a color constant that can be used? If not, can we create one?
21	We should use a const for it.
27	Is there a way to avoid negative margin? Why it is needed?
web/modals/chat/toggle-pin-modal.react.js
36–43	This might be a bit hard to maintain - the same condition is repeated a couple of times. I think it would be better if we had two objects with `name`, `action`, `confirmationText`, etc. and we choose one or another, based on a flag.
50–62	It feels like we should have an intermediate layer which splits a description of message props from its appearance. By this I mean that we could introduce an object with e.g. `isLeftAligned`, `hasHeader`, etc. For an ordinary message we would then have `{ isLeftAligned: !isViewer, hasHeader: startsConversation, ...}`, and for this message `{ isLeftAligned: true, hasHeader: true, ...}`. This refactoring might touch a couple of places, so we can postpone it, but overall I think it will be beneficial.
69–81	Immediately invoked functions reduce readability... in this case we should be able to easily avoid it.
82	So we're closing the popup before the response from the server, right?
web/utils/tooltip-utils.js
510–514	I think this might go really wrong. By introducing a new context we're splitting the data into two source of truth - which means that any modification to one won't be reflected in the other. Can we avoid this? Is the whole state needed for a message? Can we move the context above the modal?

This revision now requires changes to proceed.Apr 12 2023, 4:46 AM

All other comments are addressed in the revision

web/modals/chat/toggle-pin-modal.react.js
50–62	Makes sense, I will take a brief look at whether it makes sense to do it now or postpone it like you suggest, and after make a Linear task to not lose track of it
82	Yeah, I thought this would be safe since we optimistically create the message as is, but I can add a follow-up / update this diff to change the behavior accordingly if needed
web/utils/tooltip-utils.js
510–514	From what I experienced, since we're essentially rendering the `Message` component within the modal, without the context, the invariant here gets triggered. In terms of moving the context above the modal, I couldn't find a good place to otherwise put it when going through the component hierarchy in dev tools (the Fragment representing the modal). I'm definitely open to suggestions though if you feel like it can be changed like you suggest

rohan attached a referenced file: F482460: Screenshot 2023-04-13 at 7.34.38 AM.png. (Show Details)Apr 12 2023, 11:36 PM

ESLint + Address Feedback

Checked the build logs, looked like ESLint warnings are now resolved, and the only cause for failure is the failing test that still needs to be addressed (this will propagate up the stack)

Screenshot 2023-04-13 at 7.47.14 AM.png (728×1 px, 188 KB)

rohan attached a referenced file: F482559: Screenshot 2023-04-13 at 7.47.14 AM.png. (Show Details)Apr 12 2023, 11:48 PM

Harbormaster failed remote builds in B18295: Diff 25073!Apr 12 2023, 11:56 PM

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

tomek added inline comments.

web/components/pinned-message.react.js
31–34 ↗	(On Diff #25073)	Are we sure about displaying this conditionally? Don't we want to always display the username for a pinned message?
web/modals/chat/toggle-pin-modal.css
3 ↗	(On Diff #25073)	We can use a shorthand here, but 3-arg shorthands seem to be confusing for people, and we probably should keep it as is.
8 ↗	(On Diff #25073)	Are we sure about this? Shouldn't we have a margin instead?
web/modals/chat/toggle-pin-modal.react.js
63–79 ↗	(On Diff #25073)	This can be slightly simplified by using early exit
82		Optimistic creation makes sense

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

rohan added inline comments.Apr 17 2023, 5:00 AM

web/components/pinned-message.react.js
31–34 ↗	(On Diff #25073)	Yeah, we always want to display the username for a pinned message. This condition is basically so we don't "double render" the username since `ComposedMessage` already will render a username in chat under a certain condition: https://github.com/CommE2E/comm/blob/master/web/chat/composed-message.react.js#L208-L209
web/modals/chat/toggle-pin-modal.css
8 ↗	(On Diff #25073)	Yeah I can change this to a margin instead, may make maintaining it easier as well

Address feedback

Harbormaster failed remote builds in B18505: Diff 25341!Apr 19 2023, 6:28 AM

@rohan, can you provide some clarification as to why you've been iterating on this stack for so long without resolving the unit test issue? Is it particularly challenging?

In D7309#223179, @ashoat wrote:

@rohan, can you provide some clarification as to why you've been iterating on this stack for so long without resolving the unit test issue? Is it particularly challenging?

I spent a little time trying to figure it out, but I'm not entirely sure what change is causing it to break the unit test yet. I decided to finish addressing feedback and getting the rest of the code up for web first, so reviewers can at least provide feedback early on so all that is left to handle is just the unit test.

I'm still not 100% on what's causing it to fail, but I'm trying to figure it out locally next

Edit: This might be a circular dependency issue when I took a look using madge. Not sure if this is the cause yet

web/chat/message.react.js > web/chat/multimedia-message.react.js > web/chat/composed-message.react.js > web/utils/tooltip-utils.js > web/modals/chat/toggle-pin-modal.react.js > web/components/pinned-message.react.js

Refactor parts of tooltip-utils.js out into tooltip-action-utils.js to resolve circular dependencies (should fix the failing unit test)