Page MenuHomePhabricator

[web] Introduce `AddMembersModal` component
ClosedPublic

Authored by jacek on Apr 15 2022, 4:34 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 3:06 PM
Unknown Object (File)
Tue, Nov 12, 4:09 AM
Unknown Object (File)
Tue, Nov 12, 3:29 AM
Unknown Object (File)
Tue, Nov 12, 3:21 AM
Unknown Object (File)
Tue, Nov 12, 12:25 AM
Unknown Object (File)
Fri, Nov 8, 1:11 AM
Unknown Object (File)
Thu, Nov 7, 11:17 PM
Unknown Object (File)
Tue, Nov 5, 3:55 AM

Details

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

tomek requested changes to this revision.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:

  1. Select some users
  2. Start searching
  3. 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.

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
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.

change overflow from scroll to auto

This revision is now accepted and ready to land.Apr 26 2022, 7:31 AM