Page MenuHomePhabricator

[web] Refactor the members modal to support any variety of role labels
ClosedPublic

Authored by rohan on Jun 1 2023, 4:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 3:27 PM
Unknown Object (File)
Sat, Nov 9, 4:03 PM
Unknown Object (File)
Sat, Nov 9, 2:57 PM
Unknown Object (File)
Sat, Nov 9, 2:01 PM
Unknown Object (File)
Sat, Nov 9, 11:33 AM
Unknown Object (File)
Sat, Nov 9, 11:32 AM
Unknown Object (File)
Sat, Nov 9, 11:32 AM
Unknown Object (File)
Sat, Nov 9, 11:31 AM

Details

Summary

In preparation to support multiple custom roles, we want to remove any hard coding of 'Admins' and 'Members' in the members modal, and instead display them based on the member's role ID. We associate the ID to the role name available in the threadInfo. Note - since the "parent admin" doesn't have a role ID associated to it and is 'approximated' by it's permissions, I check for that first still.

Test Plan

Please see the screenshots indicating how it will look for custom roles (based on the Figma, we don't display 'Members', so that's why they're excluded in the code, but for testing I show them in the SS)

Screenshot 2023-06-01 at 6.03.02 PM.png (1×1 px, 97 KB)

Screenshot 2023-06-01 at 6.02.51 PM.png (1×998 px, 86 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan requested review of this revision.Jun 1 2023, 4:36 PM

Rename "Parent admin" to "Admins" [discussed in DES-44]

web/modals/threads/members/member.react.js
150

Instead of passing in role and roles, why don't we just pass in roleName?

web/modals/threads/members/member.react.js
150

roleName is defined in this useMemo block so the commit hooks catch it as

Cannot resolve name `roleName`. [cannot-resolve-name]

I could alternatively define roleName outside the scope of this memo and then put it in the dependency array

web/modals/threads/members/member.react.js
150

I could alternatively define roleName outside the scope of this memo and then put it in the dependency array

Yes that's what I'm suggesting. The fewer, more specific things you have in a dep array, the better – especially when you have the opportunity to use a primitive instead of an object (which can help avoid unnecessary recomputations)

atul requested changes to this revision.Jun 6 2023, 8:15 AM

I could alternatively define roleName outside the scope of this memo and then put it in the dependency array

Requesting changes for this

This revision now requires changes to proceed.Jun 6 2023, 8:15 AM
rohan added a subscriber: ted.

Spoke with @ted after some updated designs, we want to show the Members role label and make the background grey

Screenshot 2023-06-07 at 2.11.49 PM.png (1×1 px, 104 KB)

atul requested changes to this revision.Jun 7 2023, 12:50 PM

Label having a bg prop that expects a CSS variable is a weird API. Is there a way we can introduce a variant prop to Label instead and style accordingly within?

web/modals/threads/members/member.react.js
147 ↗(On Diff #27526)

Personally think passing CSS variable via bg prop is a very weird API, is there a way we can introduce a variant prop to Label instead and style within Label?

This revision now requires changes to proceed.Jun 7 2023, 12:50 PM

Use a variant prop to style the label. Did a quick search prior to
confirm no other instances of Label pass in a custom color or bg
prop, so it would be safe to remove. Also confirmed the existing uses of
Label still look the same

web/modals/threads/members/member.react.js
141–144 ↗(On Diff #27550)

I think the plan was to stop listing parent admins in the member list, but I'm not sure if that plan has changed, or if you're going to address it later

web/modals/threads/members/member.react.js
141–144 ↗(On Diff #27550)

Addressed in D8072. It might make sense to preemptively remove this check here, since it'll no longer be needed after the whole stack is landed

Preemptively remove the check for a parent admin. Tested with the changes
made in D8072

This revision is now accepted and ready to land.Jun 8 2023, 2:03 PM