Page MenuHomePhabricator

[web] Introduce `AddMembersList` component
ClosedPublic

Authored by jacek on Apr 15 2022, 4:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 26, 5:42 AM
Unknown Object (File)
Tue, Nov 26, 4:20 AM
Unknown Object (File)
Tue, Nov 26, 2:45 AM
Unknown Object (File)
Mon, Nov 25, 4:56 PM
Unknown Object (File)
Wed, Nov 13, 12:48 PM
Unknown Object (File)
Wed, Nov 13, 1:13 AM
Unknown Object (File)
Fri, Nov 8, 1:09 AM
Unknown Object (File)
Fri, Nov 8, 12:41 AM

Details

Summary

Introduce component grouping users and rendering list based on search results in AddMembersModal.
I used common logic with native app, where only users from parent thread have "notice" set to "undefined". It's the reason, why after grouping the title: "Users in parent thread" is added manually.

Screenshot_Google Chrome_2022-04-15_134858.png (803×457 px, 37 KB)

Test Plan

The list can be tested after introducing AddMembersModal

Diff Detail

Repository
rCOMM Comm
Branch
jacek/add-m-3
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Apr 15 2022, 9:22 AM
tomek added inline comments.
web/modals/threads/members/add-members-list.react.js
20–21

We don't memoize these so all the following memoizations will recompute after each render.

37–45

I think this code might become more readable when:

  1. We prepare all the data before this memo e.g.
_toPairs(groupedAvailableUsersList)
        .filter(group => group[0] !== 'undefined')
        .sort((a, b) => a[0].localeCompare(b[0]))
  1. We store simple values instead of creating arrays just to destructure them (see suggested edit)
This revision now requires changes to proceed.Apr 15 2022, 9:22 AM

Add missing useMemo and make the component more readable

tomek added a reviewer: ashoat.

Adding @ashoat as this diff affects the copy presented to users.

web/modals/threads/members/add-members-list.react.js
13 ↗(On Diff #11778)
21 ↗(On Diff #11778)

Shouldn't we use alertText?

32 ↗(On Diff #11778)

Just wondering, maybe we can change the logic a bit and replace undefined by something defined, so that it's more maintainable?

web/modals/threads/members/add-members-list.react.js
13 ↗(On Diff #11778)

Can probably use $ReadOnlySet<string> here instead?

Fixes after review & capitalize headers

This revision is now accepted and ready to land.Apr 25 2022, 10:10 AM