Context: https://linear.app/comm/issue/ENG-6950/update-rolepanelentrypanel-to-use-specialrole-field
We introduce this hook so we can get rid of "sketchy" string check in RolePanelEntry.
Depends on D11204
Details
Details
- Log value of specialRole in RolePanelEntry to ensure it's being passed in properly
- Look at RolePanelEntry before/after and ensure that things continue to look as expected (will add screenshots shortly)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I don't understand the reason behind this diff and the task from the summary doesn't have any description. Why can't we simply check RoleInfo and its specialRole field?
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1703–1720 ↗ | (On Diff #37729) | What is the reason behind making it a hook? It doesn't use any other hooks, so it can be an ordinary function. Also, shouldn't the returned value be memoized? |
| native/roles/role-panel-entry.react.js | ||
| 26–27 ↗ | (On Diff #37729) | Can we have a RoleInfo here instead and check its specialRole field? |
| native/roles/role-panel-entry.react.js | ||
|---|---|---|
| 26–27 ↗ | (On Diff #37729) | I completely agree that having RoleInfo as a prop for RolePanelEntry makes a ton of sense. However, I decided to match the pattern used for the rolePermissions and memberCount props where things are computed on a per-thread basis by the useRoleMemberCountsForCommunity and useRoleUserSurfacedPermissions hooks in CommunityRolesScreen and then passed into RolePanelEntry "keyed" by roleName. If I were to refactor this component to accept RoleInfo, I'd also need to refactor useRoleMemberCountsForCommunity and useRoleUserSurfacedPermissions. I'd then need to push the refactored hooks into RolePanelEntry and make necessary changes in CommunityRolesScreen to pass down RoleInfos. I think these changes would make CommunityRolesScreen and RolePanelEntry cleaner and easier to reason about, but refactoring these components is outside the scope of the task. |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1703–1720 ↗ | (On Diff #37729) | The reason for making this a hook is to avoid recomputing roleNamesToSpecialRole if the threadInfo doesn't change. roleNamesToSpecialRole is in the dep list of rolePanelList, so maintaining referential equality should reduce the number of times that components gets re-rendered. Could we have made this a plain old utility function and memoized the results using React.useMemo in CommunityRolesScreen since it's only dependent on threadInfo? Definitely, but wanted to match the pattern used by useRoleMemberCountsForCommunity and useRoleUserSurfacedPermissions
Yes! Thanks for catching that, will update this diff. |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1703–1720 ↗ | (On Diff #37729) | After introducing memoization it makes sense for it to be a hook, thanks! |
| native/roles/role-panel-entry.react.js | ||
| 26–27 ↗ | (On Diff #37729) |
Ok, fair enough |