Page MenuHomePhabricator

[web] Display list of pending users to add in `AddMembersModal`
ClosedPublic

Authored by jacek on Apr 29 2022, 5:01 AM.
Tags
None
Referenced Files
F3532340: D3876.id12325.diff
Wed, Dec 25, 9:11 AM
F3532339: D3876.id12335.diff
Wed, Dec 25, 9:11 AM
F3532338: D3876.id12113.diff
Wed, Dec 25, 9:11 AM
F3532317: D3876.id.diff
Wed, Dec 25, 9:11 AM
F3532294: D3876.diff
Wed, Dec 25, 9:06 AM
Unknown Object (File)
Mon, Dec 9, 4:14 AM
Unknown Object (File)
Wed, Dec 4, 4:32 AM
Unknown Object (File)
Nov 24 2024, 5:25 AM

Details

Summary

Introduce list of pending users to add below search bar in AddMembersModal.
The diff also excludes pending users from main list in Modal, so they don't appear twice.

Test Plan

Open "Members Modal", try to add users from list, pending users appears below search bar.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added a reviewer: ashoat.

This looks amazing! Adding @ashoat so he can take a look.

web/modals/threads/members/add-members-modal.react.js
117 ↗(On Diff #12113)

In some places we use Array.from(pendingUsersToAdd) and here we use spread operator. It doesn't matter that much, but it might be a nice idea to be consistent

130 ↗(On Diff #12113)

Have you considered using a callback here?

web/modals/threads/members/add-members-modal.react.js
117 ↗(On Diff #12113)

You're right. In many other places, I used Array.from when converting set to an array. I'll replace it to be consistent

130 ↗(On Diff #12113)

Yes, I was considering it, but I concluded it's not necessary here, as it's all inside the useMemo.

Looks great!!

web/modals/threads/members/add-members-modal.react.js
117 ↗(On Diff #12113)

I personally like the spread operator but it's all okay

130 ↗(On Diff #12113)

@def-au1t's reasoning makes sense to me

This revision is now accepted and ready to land.Apr 30 2022, 4:18 PM

replace spread with Array.from & rebase before landing