Details

Reviewers

atul
ginsu
tomek
inka

Commits

rCOMM03eb9bbf2bcf: [web] Allow interaction with inline engagement from the thread pinned messages…

Summary

When a user interacts with the inline engagement in the modal displaying pinned messages for the thread, we need to handle two cases: 1) keep the modal in the background when the reactions list is displayed, and 2) pop the modal when the replies engagement is clicked to navigate to the thread. The first is already handled by the nature of pushModal and popModal, but we need to handle the second one.

This diff pops the modal (if a modal exists) before navigating to the thread. This will also be useful for the message search implementation since I think the designs are closely shared (not too certain about it though).

Linear: https://linear.app/comm/issue/ENG-3450/allow-interaction-with-inline-engagement-from-thread-pinned-messages

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

Depends on D7380

Test Plan

Before (Modal)

After (Modal)

After (Chat)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Apr 11 2023, 6:09 AM

Herald added a subscriber: ashoat. · View Herald TranscriptApr 11 2023, 6:09 AM

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

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

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

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

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

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

Harbormaster failed remote builds in B18230: Diff 24966!

Unblocking review since the failed test is from D7309 and I'm working on figuring that out (after that, I can fix the eslint warnings)

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

rohan added a reviewer: inka.

For now it is ok, but in the future we probably should introduce a prop that is e.g. a flag and tells InlineEngagement what should be done in reaction to the click. There might be some cases, in the future, where closing the modal is not desirable.

This revision is now accepted and ready to land.Apr 12 2023, 5:43 AM

ashoat added inline comments.Apr 12 2023, 6:49 AM

web/chat/inline-engagement.react.js
52–54 ↗	(On Diff #24966)	"Replies" isn't great naming... we typically use that terminology for the "quote-based" replies in the app, in contrast with "thread". Can we maybe do this?

Also ESLint errors are concerning, we should get those resolved

In D7382#220270, @ashoat wrote:

Also ESLint errors are concerning, we should get those resolved

Will do - ran yarn eslint:fix web/ for each of the diffs in this stack, so the ESLint errors should be resolved in the updates (I'll double check this once I put up the revisions)

web/chat/inline-engagement.react.js
52–54 ↗	(On Diff #24966)	Yeah makes sense, I'll update this

ESLint + Address Feedback

Harbormaster failed remote builds in B18298: Diff 25076!Apr 12 2023, 11:57 PM

In D7382#220558, @rohan wrote:

In D7382#220270, @ashoat wrote:

Also ESLint errors are concerning, we should get those resolved

Will do - ran yarn eslint:fix web/ for each of the diffs in this stack, so the ESLint errors should be resolved in the updates (I'll double check this once I put up the revisions)