Page MenuHomePhabricator

[web] Make `AddMembersList` more universal
ClosedPublic

Authored by jakub on Sep 20 2022, 1:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 7:34 PM
Unknown Object (File)
Feb 19 2024, 2:31 PM
Unknown Object (File)
Feb 19 2024, 2:30 PM
Unknown Object (File)
Feb 19 2024, 1:31 PM
Unknown Object (File)
Feb 19 2024, 1:07 PM
Unknown Object (File)
Feb 19 2024, 11:34 AM
Unknown Object (File)
Feb 19 2024, 11:34 AM
Unknown Object (File)
Feb 19 2024, 9:38 AM

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jakub edited the test plan for this revision. (Show Details)
jakub added reviewers: tomek, atul.
jakub edited the summary of this revision. (Show Details)
jakub edited the summary of this revision. (Show Details)
jakub edited the test plan for this revision. (Show Details)

Rebase

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

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

Change MemberGroupItem types and memoize sortedGroupedUsersList

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

Memoize sortedGroupedUsersList

This revision is now accepted and ready to land.Sep 27 2022, 1:38 AM