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.
Details
Details
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)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/modals/threads/members/member.react.js | ||
|---|---|---|
| 150 ↗ | (On Diff #27388) | Instead of passing in role and roles, why don't we just pass in roleName? |
| web/modals/threads/members/member.react.js | ||
|---|---|---|
| 150 ↗ | (On Diff #27388) | 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 ↗ | (On Diff #27388) |
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) |
Comment Actions
I could alternatively define roleName outside the scope of this memo and then put it in the dependency array
Requesting changes for this
Comment Actions
Spoke with @ted after some updated designs, we want to show the Members role label and make the background grey
Comment Actions
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? |
Comment Actions
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 |
Comment Actions
Preemptively remove the check for a parent admin. Tested with the changes
made in D8072