Details

Reviewers

atul
tomek
• jacek
• abosh
ashoat

Commits

rCOMM3263b7e9a3f0: [web] [feat] [ENG-991] add modal prompt

Summary

add modal to web to prompt user about promote thread.

Image 2022-04-28 at 1.14.21 PM.jpg (686×1 px, 66 KB)

https://linear.app/comm/issue/ENG-1044/fix-regression-introduced-in-eng-972-justify-modal-header-content there's an issue with the modal header documented here. There's a little too much padding here.

Test Plan

click promote thread option. Test promote functionality and cancel promote functionality

Diff Detail

Repository

rCOMM Comm

Branch

04-27-eng-991_web_feat_add_modal_prompt

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• benschac created this revision.Apr 21 2022, 9:54 AM

Herald added subscribers: • abosh, • karol, • adrian and 2 others. · View Herald TranscriptApr 21 2022, 9:54 AM

• benschac edited the summary of this revision. (Show Details)Apr 21 2022, 9:56 AM

• benschac added reviewers: tomek, • jacek, • abosh.

Harbormaster completed remote builds in B8409: Diff 11734.Apr 21 2022, 10:02 AM

• benschac requested review of this revision.Apr 21 2022, 10:02 AM

• benschac added a parent revision: D3810: [web] [feat] [ENG-991] add show promote thread logic to web.Apr 21 2022, 10:10 AM

• benschac retitled this revision from [web] [feat] add modal prompt to [web] [feat] [ENG-991] add modal prompt.

• benschac edited the summary of this revision. (Show Details)

change onCancel fn

Harbormaster completed remote builds in B8413: Diff 11738.Apr 21 2022, 10:19 AM

I just noticed modal styling broke during a rebase

Broken in production or just in your stack? If that latter, might be better to hold off on submitting this diff until you have your stack straightened out.

I just noticed modal styling broke during a rebase.

Are you planning changes to this diff, or did you want it to be reviewed as is?

In D3809#105846, @ashoat wrote:

I just noticed modal styling broke during a rebase

Broken in production or just in your stack? If that latter, might be better to hold off on submitting this diff until you have your stack straightened out.

Production has been broken for quite some time, this is just in the stack. The stack will get fixed. I'm not going to land it without updated styling.

In D3809#106403, @atul wrote:

I just noticed modal styling broke during a rebase.

Are you planning changes to this diff, or did you want it to be reviewed as is?

Reviewed as is. I'll follow up with styling changes in a future diff, since they're difference concerns being addressed.

I'm wondering if, instead of creating separate SidebarPromoteModal component, shouldn't we use existing logic already implemented in ConfirmLeaveThreadModal and create universal ConfirmModal component, that can receive e.g. title, message (content), and confirm action name? I think, there are quite many places, where something like this could be used in web app. What do you think?

web/chat/thread-menu.react.js
100–111 ↗	(On Diff #11738)	I think it should be defined closer to where it's used. Every item is next to its callback in this file.
web/modals/chat/sidebar-promote-modal.react.js
13 ↗	(On Diff #11738)	It probably doesn't matter much, but I'm wondering if it's better to pass the whole `ThreadInfo` object in the props, or only the value, that is used (`uiName`). Passing more complex object makes the component rerender more often due to props changes, but from the other side, it's harder to make mistake by passing wrong type accidentally. @palys-swm what approach is better?
31–36 ↗	(On Diff #11738)	I think the sequence should be reversed here. I'm not sure if we have a Figma design for the modal, but in all modals I've seen, confirming action (marked with primary, or danger button) is on the right, and the secondary cancel button is on the left:

This revision now requires changes to proceed.Apr 26 2022, 4:33 AM

In D3809#106845, @def-au1t wrote:

I'm wondering if, instead of creating separate SidebarPromoteModal component, shouldn't we use existing logic already implemented in ConfirmLeaveThreadModal and create universal ConfirmModal component, that can receive e.g. title, message (content), and confirm action name? I think, there are quite many places, where something like this could be used in web app. What do you think?

I like the idea, maybe we could still implement it here and after getting this diff in then create a ConfirmModal component that will get used across the app.

diff review fixes.

• benschac added inline comments.Apr 26 2022, 10:42 AM

web/chat/thread-menu.react.js
100–111 ↗	(On Diff #11738)	updated
web/modals/chat/sidebar-promote-modal.react.js
13 ↗	(On Diff #11738)	I don't think it's that big of a deal since this modal isn't persisted on the page. I was just following what the convention was in the codebase. Happy to update or leave it, I don't have a strong opinion either way. curious what @palys-swm thinks.
31–36 ↗	(On Diff #11738)	That's what's here now! I agree the HTML order is a bit misleading. We should clean up the modal CSS a bit more.

Harbormaster completed remote builds in B8572: Diff 11951.Apr 26 2022, 10:46 AM

Adding @ashoat as the copy is updated

web/modals/chat/sidebar-promote-modal.react.js
29 ↗	(On Diff #11951)	This doesn't sound great, but maybe it's just me
13 ↗	(On Diff #11738)	That's true that we're using only `uiName`, so passing only this sounds like a good idea. On the other hand, passing the whole `threadInfo` also makes sense and better expresses the intention - we're upgrading the whole thread. Although the modal isn't persisted, when we use `uiName` then the dependencies of `onClickPromoteSidebarToThread` would change, which is beneficial. So overall, I don't have a strong opinion. Really slightly prefer passing just the name, but don't think it's worth breaking the consistency.
31–36 ↗	(On Diff #11738)	Agree, we should improve the maintainability here

I'm wondering if, instead of creating separate SidebarPromoteModal component, shouldn't we use existing logic already implemented in ConfirmLeaveThreadModal and create universal ConfirmModal component, that can receive e.g. title, message (content), and confirm action name? I think, there are quite many places, where something like this could be used in web app. What do you think?

Ah there's a task for something like this: https://linear.app/comm/issue/ENG-1003/[web]-introduce-prompt-component. It's taken me longer to get to it than I'd have hoped

• jacek requested changes to this revision.Apr 27 2022, 2:08 AM

• jacek added inline comments.

web/modals/chat/sidebar-promote-modal.react.js
29 ↗	(On Diff #11951)	I think, there was already a language issue in this sentence: https://linear.app/comm/issue/ENG-1016/add-prompt-to-user-that-sidebar-promotion-cant-be-undone https://phabricator.ashoat.com/D3755#inline-22663 https://phabricator.ashoat.com/D3819
31–36 ↗	(On Diff #11738)	It's a different component on the screenshot you sent - `ConfirmLeaveThreadModal`, isn't it? The sequence used there is reversed - https://github.com/CommE2E/comm/blob/master/web/modals/threads/confirm-leave-thread-modal.react.js#L34-L41 Shouldn't it be the same order here, or am I missing something?

This revision now requires changes to proceed.Apr 27 2022, 2:08 AM

rebase fix

• benschac planned changes to this revision.Apr 27 2022, 8:24 AM

• benschac added inline comments.

web/modals/chat/sidebar-promote-modal.react.js
29 ↗	(On Diff #11951)	@palys-swm @def-au1t -- I should have caught this copy beforehand. 100% this needs to be changed.

Harbormaster completed remote builds in B8607: Diff 11991.Apr 27 2022, 8:27 AM

fix modal

remove file

revert

Harbormaster completed remote builds in B8655: Diff 12046.Apr 28 2022, 10:23 AM

remove file

Harbormaster completed remote builds in B8656: Diff 12047.Apr 28 2022, 10:26 AM

Harbormaster completed remote builds in B8657: Diff 12048.

Harbormaster completed remote builds in B8658: Diff 12049.Apr 28 2022, 10:30 AM

• benschac planned changes to this revision.Apr 28 2022, 10:49 AM

• benschac edited the summary of this revision. (Show Details)

change copy

• benschac added inline comments.Apr 28 2022, 11:00 AM

web/modals/chat/sidebar-promote-modal.react.js
35 ↗	(On Diff #12051)	took this copy from `thread-settings-promote-sidebar.react` in `native`. cc: @ashoat if you think we should use different copy on web.
13 ↗	(On Diff #11738)	I'm going to just leave the `threadInfo` for now then if there's no strong preference.
31–36 ↗	(On Diff #11738)	nope, this is my fault. I was looking at the wrong component. I've edited and screen shot the correct one. Thanks for catching this.