Details

Reviewers

atul
ginsu

Commits

rCOMM4755e04a8254: [native] Create the community roles screen

Summary

This diff handles creating the screen that will display all roles in a given community, alongside a count of how many members are assigned each role. There is also a button that will allow admins to create a new role - the functionality of the button will be implemented in the next diff. The designs for this screen can be found on the Figma.

Note: For now, I pull the threadInfo directly from the navigation params, but in a later diff I plan to pull the updated threadInfo from the redux store (a solution discussed in my 1:1 with Ashoat) in order to make sure the roles list is always kept up to date as new roles are added and users are navigated back to this screen. This will need to be done after the screen for creating a custom role is complete, so that's why it's going to be implemented later.

Depends on D8358

ENG-4168.

Test Plan

Confirm that for communities, the correct roles and role counts show up in the new screen. Also verified that for communities with several custom roles, the scroll container looks fine.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Jun 28 2023, 9:48 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 28 2023, 9:48 AM

rohan edited the summary of this revision. (Show Details)Jun 28 2023, 9:52 AM

rohan edited the test plan for this revision. (Show Details)

rohan attached a referenced file: F610727: Simulator Screen Recording - iPhone 14 Pro - 2023-06-28 at 12.51.24.mp4. (Show Details)

Harbormaster completed remote builds in B20578: Diff 28213.Jun 28 2023, 10:05 AM

rohan requested review of this revision.Jun 28 2023, 10:05 AM

Refactor role name & member counts extraction logic to a helper function in lib so it can be reused on web later

Harbormaster completed remote builds in B20579: Diff 28214.Jun 28 2023, 10:22 AM

Couple of suggestions inline. Only one that's blocking this diff from being accepted is the use of \n\n for layout, let's break that text apart and use eg padding/margin/etc to lay things out properly.

lib/shared/thread-utils.js
1592 ↗	(On Diff #28214)	I wonder if `getRoleMemberCounts` or `getRoleMemberCountsForThread` would be a more precise name?
1597–1613 ↗	(On Diff #28214)	Not a huge deal, but could we include mapping roleIDs to roleNames within this loop to simplify things?
1601 ↗	(On Diff #28214)	Should we annotate type here to improve readability a bit?
1603–1613 ↗	(On Diff #28214)	Could this loop be simplified to something like: threadInfo.members.forEach(({ role: roleID }) => { invariant(roleID, 'Community member should have a role'); const roleName = roleIDsToNames[roleID]; roleNamesToMemberCount[roleName] = (roleNamesToMemberCount[roleName] ?? 0) + 1; });
native/roles/community-roles-screen.react.js
39–45 ↗	(On Diff #28214)	When I see a big clump of JSX being pushed to a list like this, I wonder if that clump of JSX should be a separate component? Whatever you think is best, clearest, easiest to maintain, etc
65 ↗	(On Diff #28214)	Using multiple newline characters (or `<br />`s) for layout is a red flag. Would be better to separate these chunks of text into separate `<Text>` components and use margin/padding/etc to space things out properly.

This revision now requires changes to proceed.Jun 28 2023, 11:02 AM

ashoat added inline comments.Jun 28 2023, 6:24 PM

lib/shared/thread-utils.js
1615 ↗	(On Diff #28214)	Typo

Address feedback

rohan added a child revision: D8379: [native] Navigate to a screen to create a new role.Jun 29 2023, 9:52 AM

Harbormaster completed remote builds in B20623: Diff 28273.Jun 29 2023, 9:59 AM

Add key to RolePanelEntry

Harbormaster completed remote builds in B20644: Diff 28298.Jun 30 2023, 8:21 AM

Change button text from Create Role to Create role

Harbormaster completed remote builds in B20696: Diff 28358.Jul 3 2023, 8:32 AM

Looks good now, thanks for addressing all the feedback!

ashoat added inline comments.Jul 6 2023, 12:31 PM

native/roles/community-roles-screen.react.js
135 ↗	(On Diff #28358)	See comment here
native/roles/role-panel-entry.react.js
37 ↗	(On Diff #28358)	See comment here
42 ↗	(On Diff #28358)	See comment here

ginsu added inline comments.Jul 6 2023, 12:38 PM

native/roles/community-roles-screen.react.js
135 ↗	(On Diff #28358)	This text will be placed on a purple background (the create role button), so this should be portable on both light/dark mode

ashoat added inline comments.Jul 6 2023, 1:06 PM

native/roles/community-roles-screen.react.js
135 ↗	(On Diff #28358)	Ah, my bad – looks like this is fine. Are the others still concerning, or are those cases similar?

Let's fix the rolePanelNameEntry color and the rolePanelCountEntry. Sorry should have caught this in my first round of review

native/roles/community-roles-screen.react.js
135 ↗	(On Diff #28358)	The others should be fixed, requesting changes for this rn

This revision now requires changes to proceed.Jul 6 2023, 1:10 PM

whiteText --> panelForegroundLabel (sorry should've caught this earlier myself)

Harbormaster completed remote builds in B20776: Diff 28455.Jul 6 2023, 1:55 PM

ginsu accepted this revision.Jul 7 2023, 8:56 AM

Reduce rolePanelList max height

Harbormaster completed remote builds in B20852: Diff 28560.Jul 10 2023, 12:25 PM

rohan mentioned this in D8593: [web] Create a community roles modal to display all roles and member counts.Jul 21 2023, 9:03 AM

atul accepted this revision.Jul 23 2023, 9:56 PM

atul added inline comments.

lib/shared/thread-utils.js
1588 ↗	(On Diff #28560)	Can we remove the `Type` suffix from this type name?
native/roles/community-roles-screen.react.js
135 ↗	(On Diff #28560)	Is this `whiteText` fine in both light and dark mode?
native/roles/role-panel-entry.react.js
20 ↗	(On Diff #28560)	Given that we're in the `role-panel-entry` component, we could probably just name this something like `name` and `styles.rolePanelCountEntry` something like `count`? Whatever naming convention you prefer, doesn't really matter