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 5, 3:55 AM
Unknown Object (File)
Sat, Nov 2, 12:29 PM
Unknown Object (File)
Sat, Nov 2, 9:39 AM
Unknown Object (File)
Wed, Oct 16, 8:41 AM
Unknown Object (File)
Wed, Oct 16, 8:41 AM
Unknown Object (File)
Wed, Oct 16, 8:41 AM
Unknown Object (File)
Wed, Oct 16, 8:40 AM
Unknown Object (File)
Wed, Oct 16, 8:40 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
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 ↗(On Diff #11511)

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

37–45 ↗(On Diff #11511)

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
21

Shouldn't we use alertText?

32

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

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