Page MenuHomePhabricator

[web] Introduce `AddMemberGroup` component
ClosedPublic

Authored by jacek on Apr 15 2022, 4:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 1:12 AM
Unknown Object (File)
Fri, Dec 20, 1:11 AM
Unknown Object (File)
Wed, Dec 18, 3:25 AM
Unknown Object (File)
Sat, Dec 14, 1:10 AM
Unknown Object (File)
Sat, Dec 14, 12:32 AM
Unknown Object (File)
Sat, Dec 7, 8:16 PM
Unknown Object (File)
Nov 13 2024, 12:48 PM
Unknown Object (File)
Nov 13 2024, 1:13 AM

Details

Summary

Introduce component displaying group of users on the list in AddMembersModal - e.g. "Users in parent thread:", or "Not in parent thread"

Screenshot_Safari_2022-04-15_131122.png (262×465 px, 14 KB)

Test Plan

Component can be tested after introducing "AddMembersModal"

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

jacek edited the test plan for this revision. (Show Details)
tomek requested changes to this revision.Apr 15 2022, 9:08 AM
tomek added inline comments.
web/modals/threads/members/add-members-group.react.js
14 ↗(On Diff #11510)

We should use set to avoid O(n^2) complexity

22 ↗(On Diff #11510)

Sorting here is the most expensive operation - maybe it makes sense to create a separate memo with just this? It will depend on just userInfos

web/modals/threads/members/members-modal.css
58–60 ↗(On Diff #11510)

It might be a good idea, but can't we just use header with correct capitalization?

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

Follow comments after review

tomek requested changes to this revision.Apr 25 2022, 6:34 AM

Looks ok, but would like to clarify header capitalization before accepting.

web/modals/threads/members/add-members-group.react.js
14
18–19

Is it possible to receive a header that doesn't need to be modified? (prop that is capitalized by parent component)

40

What do you think about template literal here?

This revision now requires changes to proceed.Apr 25 2022, 6:34 AM
web/modals/threads/members/add-members-group.react.js
40

By template literal do you mean something like:

<div className={css.addMemberItemsGroupHeader}>{`${capitalizedHeader}:`}</div>

?

I'm not sure if it's more readable as we need more braces.

fix type and move capitalizing header into parent component

tomek added inline comments.
web/modals/threads/members/add-members-group.react.js
40

Ok, let's keep it

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