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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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 ↗	(On Diff #11512)
23 ↗	(On Diff #11512)	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 ↗	(On Diff #11512)	We should prefer using `set` for performance reasons
51–52 ↗	(On Diff #11512)	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 ↗	(On Diff #11512)	I guess this will be implemented later
web/modals/threads/members/members-modal.css
73–75 ↗	(On Diff #11512)	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`.