Page MenuHomePhabricator
Differential D5185

[web] Make `AddMembersList` more universal
ClosedPublic
Actions

Authored by • jakub on Sep 20 2022, 1:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 9:25 PM
Unknown Object (File)
Fri, Nov 22, 9:21 PM
Unknown Object (File)
Fri, Nov 22, 8:48 PM
Unknown Object (File)
Wed, Nov 20, 11:08 AM
Unknown Object (File)
Sat, Nov 9, 3:18 PM
Unknown Object (File)
Sat, Nov 9, 2:34 PM
Unknown Object (File)
Sat, Nov 9, 2:33 PM
Unknown Object (File)
Sat, Nov 9, 1:40 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
tomek
atul
rohan
ginsu
Commits
rCOMMd1d82f4ef167: [web] Make `AddMembersList` more universal
Summary

Depends on D5115
AddMembersList component could be reused in another modals (i.a. in next diff - ComposeSubchannelModal)

Main changes:

a) split AddMembersList to 2 components:

  1. AddMembersListContent- contains logic responsible directly for data processing for Members modal
  2. AddMembersList - generate list

b) create directory for common modal components (web/modals/components)

c) move all associated with current AddMembersList components to web/modals/components

Test Plan
  1. Open any chat on web and open Members modal
  2. Click "Add members"

AddMemberList works as before

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jakub created this revision.Sep 20 2022, 1:12 AM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptSep 20 2022, 1:12 AM
Harbormaster completed remote builds in B12296: Diff 16881.Sep 20 2022, 1:29 AM
• jakub requested review of this revision.Sep 20 2022, 1:29 AM
• jakub edited the summary of this revision. (Show Details)Sep 20 2022, 2:12 AM
• jakub edited the test plan for this revision. (Show Details)
• jakub added reviewers: tomek, atul.
• jakub added a parent revision: D5115: [web] Added possibility to change input type and icon position in EnumSettingsOption.
• jakub edited the summary of this revision. (Show Details)
• jakub edited the summary of this revision. (Show Details)
• jakub added a child revision: D5186: [web] Introduce `Stepper` - common multiple step component.Sep 20 2022, 2:30 AM
• jakub updated this revision to Diff 16924.Sep 20 2022, 11:32 AM
• jakub edited the test plan for this revision. (Show Details)

Rebase

Harbormaster completed remote builds in B12331: Diff 16924.Sep 20 2022, 11:42 AM
atul added reviewers: rohan, ginsu.Sep 21 2022, 8:41 AM
tomek requested changes to this revision.Sep 26 2022, 3:46 AM

Usually when you can list a couple of things that a diff does, it is a good indication that there should be multiple diffs. In this case, creating new directory and moving there should be the first diff and splitting should be the second one. But I don't think it makes sense to split it now.

web/modals/components/add-members-list.react.js
8–9 ↗(On Diff #16924)

We should almost never use any. If we don't know what will be there and it depends on the usage, we should use generic types. If we don't care what's there, we should use mixed. But in this case it might be a good idea to have actual types.

web/modals/threads/members/add-members-list-content.react.js
78–80 ↗(On Diff #16924)

We don't need to generate new list on every render, so maybe it is a good idea to memoize

sortedGroupedUsersList.map(
        ([header, userInfos]) => ({ header, userInfos }),
)
This revision now requires changes to proceed.Sep 26 2022, 3:46 AM
ashoat added a comment.Sep 26 2022, 4:51 AM

In this case, creating new directory and moving there should be the first diff and splitting should be the second one.

More on this: https://www.notion.so/commapp/Moving-code-around-bbb551c4350b4d5cb553c6751be44314

• jakub updated this revision to Diff 17080.Sep 26 2022, 7:01 AM

Change MemberGroupItem types and memoize sortedGroupedUsersList

Harbormaster completed remote builds in B12437: Diff 17080.Sep 26 2022, 7:11 AM
atul accepted this revision.Sep 26 2022, 10:21 AM
atul added inline comments.
web/modals/components/add-members-list.react.js
23–31 ↗(On Diff #17080)

Can we memoize this and return?

eg

const addMembersList = React.useMemo(() => {
  return sortedGroupedUsersList.map(({ header, userInfos })....
};

return addMembersList;
web/modals/components/add-members.css
2–3 ↗(On Diff #17080)

Nice catch using the CSS custom properties here

• jakub updated this revision to Diff 17090.Sep 26 2022, 11:38 AM

Memoize sortedGroupedUsersList

Harbormaster completed remote builds in B12444: Diff 17090.Sep 26 2022, 11:48 AM
tomek accepted this revision.Sep 27 2022, 1:38 AM
This revision is now accepted and ready to land.Sep 27 2022, 1:38 AM
• jakub updated this revision to Diff 17095.Sep 27 2022, 2:25 AM

Rebase before landing

Harbormaster completed remote builds in B12449: Diff 17095.Sep 27 2022, 2:34 AM
Closed by commit rCOMMd1d82f4ef167: [web] Make `AddMembersList` more universal (authored by • jakub). · Explain WhySep 27 2022, 2:38 AM
This revision was automatically updated to reflect the committed changes.
• jakub added a commit: rCOMMd1d82f4ef167: [web] Make `AddMembersList` more universal.

Revision Contents
Changeset List

Diff 17090

View Options

web/modals/components/add-members-group.react.js

Loading...
View Options

web/modals/components/add-members-item.react.js

Loading...
View Options

web/modals/components/add-members-list.react.js

Loading...
View Options

web/modals/components/add-members.css

Loading...
View Options

web/modals/threads/members/add-members-group.react.js

Loading...
View Options

web/modals/threads/members/add-members-item.react.js

Loading...
View Options

web/modals/threads/members/add-members-list-content.react.js

Loading...
View Options

web/modals/threads/members/add-members-list.react.js

Loading...
View Options

web/modals/threads/members/add-members-modal.react.js

Loading...
View Options

web/modals/threads/members/members-modal.css

Loading...