Page MenuHomePhabricator
Differential D3809

[web] [feat] [ENG-991] add modal prompt
ClosedPublic
Actions

Authored by • benschac on Apr 21 2022, 9:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 23, 4:33 PM
Unknown Object (File)
Sat, Apr 19, 1:53 PM
Unknown Object (File)
Sat, Apr 19, 12:03 PM
Unknown Object (File)
Sat, Apr 19, 11:11 AM
Unknown Object (File)
Sat, Apr 19, 5:53 AM
Unknown Object (File)
Sat, Apr 19, 5:06 AM
Unknown Object (File)
Sat, Apr 19, 3:34 AM
Unknown Object (File)
Sat, Apr 19, 2:56 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
• karol
tomek

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
benjamin/eng-991-promote-sidebar-to-subthread-on-web
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)
• benschac updated this revision to Diff 11738.Apr 21 2022, 10:12 AM

change onCancel fn

Harbormaster completed remote builds in B8413: Diff 11738.Apr 21 2022, 10:19 AM
ashoat added a comment.Apr 22 2022, 6:13 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.

atul added a comment.Apr 25 2022, 7:15 AM

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?

• benschac added a comment.Apr 25 2022, 8:28 AM
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.

• benschac added a comment.Apr 25 2022, 8:28 AM
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.

• jacek requested changes to this revision.Apr 26 2022, 4:33 AM

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:

Screenshot_Figma_2022-04-26_131800.png (284×607 px, 20 KB)

This revision now requires changes to proceed.Apr 26 2022, 4:33 AM
• benschac added a comment.Apr 26 2022, 10:17 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.

• benschac updated this revision to Diff 11951.Apr 26 2022, 10:35 AM

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!

Image 2022-04-26 at 1.24.42 PM.jpg (476×1 px, 46 KB)

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
tomek accepted this revision.Apr 26 2022, 11:04 AM
tomek added a reviewer: ashoat.

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

atul added a comment.Apr 26 2022, 4:45 PM

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
• benschac updated this revision to Diff 11991.Apr 27 2022, 8:22 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
• benschac updated this revision to Diff 12046.Apr 28 2022, 10:18 AM

fix modal

• benschac updated this revision to Diff 12047.Apr 28 2022, 10:20 AM

remove file

• benschac updated this revision to Diff 12048.Apr 28 2022, 10:21 AM

revert

Harbormaster completed remote builds in B8655: Diff 12046.Apr 28 2022, 10:23 AM
• benschac updated this revision to Diff 12049.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)
• benschac updated this revision to Diff 12051.Apr 28 2022, 10:57 AM

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.

Harbormaster completed remote builds in B8660: Diff 12051.Apr 28 2022, 11:02 AM
ashoat accepted this revision.Apr 28 2022, 11:52 AM

Looks good

• jacek accepted this revision.Apr 29 2022, 2:35 AM
This revision is now accepted and ready to land.Apr 29 2022, 2:35 AM
• benschac updated this revision to Diff 12119.Apr 29 2022, 10:21 AM

Fix formatting of content, remove unneeded quotes

Harbormaster completed remote builds in B8700: Diff 12119.Apr 29 2022, 10:26 AM
Closed by commit rCOMM3263b7e9a3f0: [web] [feat] [ENG-991] add modal prompt (authored by • benschac). · Explain WhyMay 4 2022, 4:41 AM
This revision was automatically updated to reflect the committed changes.
• benschac added a commit: rCOMM3263b7e9a3f0: [web] [feat] [ENG-991] add modal prompt.

Revision Contents
Changeset List

PathSize
web/
chat/
17 lines
modals/
chat/
42 lines

Diff 11734

View Options

web/chat/thread-menu.react.js

// @flow // @flow
import invariant from 'invariant'; import invariant from 'invariant';
import type { Shape } from './core.js'; import type { Shape } from './core.js';
import type { CalendarQuery, RawEntryInfo } from './entry-types.js'; import type { CalendarQuery, RawEntryInfo } from './entry-types.js';
import type { Media } from './media-types.js';
import type { import type {
RawMessageInfo, RawMessageInfo,
MessageTruncationStatuses, MessageTruncationStatuses,
} from './message-types.js'; } from './message-types.js';
import type { ThreadSubscription } from './subscription-types.js'; import type { ThreadSubscription } from './subscription-types.js';
import type { ServerUpdateInfo, ClientUpdateInfo } from './update-types.js'; import type { ServerUpdateInfo, ClientUpdateInfo } from './update-types.js';
import type { UserInfo, UserInfos } from './user-types.js'; import type { UserInfo, UserInfos } from './user-types.js';
import type { ThreadEntity } from '../utils/entity-text.js'; import type { ThreadEntity } from '../utils/entity-text.js';
▲ Show 20 Lines • Show All 433 Lines • ▼ Show 20 Linesexport type ThreadJoinPayload = {
+updatesResult: { +updatesResult: {
newUpdates: $ReadOnlyArray<ClientUpdateInfo>, newUpdates: $ReadOnlyArray<ClientUpdateInfo>,
}, },
+rawMessageInfos: $ReadOnlyArray<RawMessageInfo>, +rawMessageInfos: $ReadOnlyArray<RawMessageInfo>,
+truncationStatuses: MessageTruncationStatuses, +truncationStatuses: MessageTruncationStatuses,
+userInfos: $ReadOnlyArray<UserInfo>, +userInfos: $ReadOnlyArray<UserInfo>,
}; };
export type ThreadFetchMediaResult = {
+media: $ReadOnlyArray<Media>,
};
export type ThreadFetchMediaRequest = {
+threadID: string,
+limit: number,
+offset: number,
};
export type SidebarInfo = { export type SidebarInfo = {
+threadInfo: ThreadInfo, +threadInfo: ThreadInfo,
+lastUpdatedTime: number, +lastUpdatedTime: number,
+mostRecentNonLocalMessage: ?string, +mostRecentNonLocalMessage: ?string,
}; };
// We can show a max of 3 sidebars inline underneath their parent in the chat // We can show a max of 3 sidebars inline underneath their parent in the chat
// tab. If there are more, we show a button that opens a modal to see the rest // tab. If there are more, we show a button that opens a modal to see the rest
export const maxReadSidebars = 3; export const maxReadSidebars = 3;
// We can show a max of 5 sidebars inline underneath their parent // We can show a max of 5 sidebars inline underneath their parent
// in the chat tab if every one of the displayed sidebars is unread // in the chat tab if every one of the displayed sidebars is unread
export const maxUnreadSidebars = 5; export const maxUnreadSidebars = 5;
View Options

web/modals/chat/sidebar-promote-modal.react.js

// @flow // @flow
import invariant from 'invariant'; import invariant from 'invariant';
import type { Shape } from './core.js'; import type { Shape } from './core.js';
import type { CalendarQuery, RawEntryInfo } from './entry-types.js'; import type { CalendarQuery, RawEntryInfo } from './entry-types.js';
import type { Media } from './media-types.js';
import type { import type {
RawMessageInfo, RawMessageInfo,
MessageTruncationStatuses, MessageTruncationStatuses,
} from './message-types.js'; } from './message-types.js';
import type { ThreadSubscription } from './subscription-types.js'; import type { ThreadSubscription } from './subscription-types.js';
import type { ServerUpdateInfo, ClientUpdateInfo } from './update-types.js'; import type { ServerUpdateInfo, ClientUpdateInfo } from './update-types.js';
import type { UserInfo, UserInfos } from './user-types.js'; import type { UserInfo, UserInfos } from './user-types.js';
import type { ThreadEntity } from '../utils/entity-text.js'; import type { ThreadEntity } from '../utils/entity-text.js';
▲ Show 20 Lines • Show All 433 Lines • ▼ Show 20 Linesexport type ThreadJoinPayload = {
+updatesResult: { +updatesResult: {
newUpdates: $ReadOnlyArray<ClientUpdateInfo>, newUpdates: $ReadOnlyArray<ClientUpdateInfo>,
}, },
+rawMessageInfos: $ReadOnlyArray<RawMessageInfo>, +rawMessageInfos: $ReadOnlyArray<RawMessageInfo>,
+truncationStatuses: MessageTruncationStatuses, +truncationStatuses: MessageTruncationStatuses,
+userInfos: $ReadOnlyArray<UserInfo>, +userInfos: $ReadOnlyArray<UserInfo>,
}; };
export type ThreadFetchMediaResult = {
+media: $ReadOnlyArray<Media>,
};
export type ThreadFetchMediaRequest = {
+threadID: string,
+limit: number,
+offset: number,
};
export type SidebarInfo = { export type SidebarInfo = {
+threadInfo: ThreadInfo, +threadInfo: ThreadInfo,
+lastUpdatedTime: number, +lastUpdatedTime: number,
+mostRecentNonLocalMessage: ?string, +mostRecentNonLocalMessage: ?string,
}; };
// We can show a max of 3 sidebars inline underneath their parent in the chat // We can show a max of 3 sidebars inline underneath their parent in the chat
// tab. If there are more, we show a button that opens a modal to see the rest // tab. If there are more, we show a button that opens a modal to see the rest
export const maxReadSidebars = 3; export const maxReadSidebars = 3;
// We can show a max of 5 sidebars inline underneath their parent // We can show a max of 5 sidebars inline underneath their parent
// in the chat tab if every one of the displayed sidebars is unread // in the chat tab if every one of the displayed sidebars is unread
export const maxUnreadSidebars = 5; export const maxUnreadSidebars = 5;