Page MenuHomePhabricator

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

Authored by rohan on Apr 4 2023, 7:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 26, 9:37 PM
Unknown Object (File)
Mon, Dec 23, 11:16 AM
Unknown Object (File)
Fri, Dec 20, 1:56 AM
Unknown Object (File)
Fri, Dec 20, 12:43 AM
Unknown Object (File)
Thu, Dec 19, 10:51 PM
Unknown Object (File)
Tue, Dec 17, 1:48 AM
Unknown Object (File)
Sat, Dec 7, 8:33 PM
Unknown Object (File)
Sat, Dec 7, 8:33 PM
Subscribers

Details

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 edited the test plan for this revision. (Show Details)
rohan edited the summary of this revision. (Show Details)
web/modals/chat/toggle-pin-modal.react.js
59–64

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

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!
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!
web/components/pinned-message.react.js
31–34 ↗(On Diff #24958)

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 ↗(On Diff #24958)

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.

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

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

tomek requested changes to this revision.Apr 12 2023, 4:46 AM

This diff causes eslint to fail. Please fix it.

web/components/pinned-message.css
6 ↗(On Diff #24958)

You can use a shorthand

20 ↗(On Diff #24958)

Do we have a color constant that can be used? If not, can we create one?

21 ↗(On Diff #24958)

We should use a const for it.

27 ↗(On Diff #24958)

Is there a way to avoid negative margin? Why it is needed?

web/modals/chat/toggle-pin-modal.react.js
36–43 ↗(On Diff #24958)

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 ↗(On Diff #24958)

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 ↗(On Diff #24958)

Immediately invoked functions reduce readability... in this case we should be able to easily avoid it.

82 ↗(On Diff #24958)

So we're closing the popup before the response from the server, right?

web/utils/tooltip-utils.js
510–514 ↗(On Diff #24958)

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 ↗(On Diff #24958)

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 ↗(On Diff #24958)

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 ↗(On Diff #24958)

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

Screenshot 2023-04-13 at 7.34.38 AM.png (538×726 px, 87 KB)

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)

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 ↗(On Diff #24958)

Optimistic creation makes sense

This revision is now accepted and ready to land.Apr 13 2023, 3:18 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

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

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

Undo the commit, I should do it in the diff below this

Rebase (expecting to see the unit test pass now) after running yarn workspace web test locally successfully

Rename PinnedMessage -> MessageResult