Details

Reviewers

tomek
atul
• benschac

Commits

rCOMMb9d09aa7006b: [web] Introduce `AddMembersModal` component

Summary

Introduce "AddMembersModal", that is used for searching and selecting users to add to a thread.
There is no logic for searching users, and for dispatching server action, added in this diff yet - they will be added in the following diffs.

https://linear.app/comm/issue/ENG-966/allow-adding-users-to-the-thread

Demo:

Test Plan

The modal will be displayed after pressing "Add members" button in MembersModal - after introducing the action in the following diffs.

Diff Detail

Repository

rCOMM Comm

Branch

jacek/add-m-4

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jacek created this revision.Apr 15 2022, 4:34 AM

Herald added subscribers: • abosh, • benschac, • karol and 4 others. · View Herald TranscriptApr 15 2022, 4:34 AM

• jacek edited the summary of this revision. (Show Details)Apr 15 2022, 4:54 AM

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

• jacek added a child revision: D3743: [web] Introduce searching users in `AddMembersModal`.Apr 15 2022, 4:57 AM

• jacek added a parent revision: D3741: [web] Introduce `AddMembersList` component.

Harbormaster completed remote builds in B8228: Diff 11512.Apr 15 2022, 8:23 AM

• jacek requested review of this revision.Apr 15 2022, 8:23 AM

One issue I can notice with this design is that it is not obvious which users will be added while searching:

Select some users
Start searching
Add users

I think it might be a good idea to either always display about to be added users, or display a confirmation modal with such list.
At this point this component is functional, so we can consider this as a followup task.

web/modals/threads/members/add-members-modal.react.js
19
23	I guess this is going to be added in next diffs. At this point the value is not memoized which breaks the whole memoization
28	We should prefer using `set` for performance reasons
51–52	I'm not sure how this button works, but it shouldn't be necessary to change its variant when disabling it. We need to fix it if that operation is necessary
53	I guess this will be implemented later
web/modals/threads/members/members-modal.css
73–75	This should be a part of `Button` - we need to move this style there

This revision now requires changes to proceed.Apr 15 2022, 9:32 AM

One issue I can notice with this design is that it is not obvious which users will be added while searching

Yeah, this is a good point...

At this point this component is functional, so we can consider this as a followup task.

Agree it makes most sense as a follow-up task.

replace array of users with set and changed button types.

• jacek added a parent revision: D3813: [web] Introduce `:disabled` and `:hover` styles for `Button`.Apr 22 2022, 3:42 AM

Harbormaster completed remote builds in B8442: Diff 11781.Apr 22 2022, 3:53 AM

ashoat added inline comments.Apr 22 2022, 11:15 AM

web/modals/threads/members/add-members-modal.react.js
20 ↗	(On Diff #11781)	I think this should be `$ReadOnlySet<string>`

tomek requested changes to this revision.Apr 26 2022, 4:14 AM

tomek added inline comments.

web/modals/threads/members/members-modal.css
64 ↗	(On Diff #11781)	We should prefer `auto` as we want the scroll bar to be visible only when necessary

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

• jacek added inline comments.Apr 26 2022, 4:44 AM

web/modals/threads/members/members-modal.css
64 ↗	(On Diff #11781)	Oh, thank you for this catch. I didn't know that, as I had "show scrollbars" set to "when scrolling" in macOS preferences. I'll put up a diff changing existing `overflow: scroll` to `overflow:auto` for my already landed diffs - in `SubchannelsModal` and `MembersModal`.