Page MenuHomePhabricator
Differential D8156

[native] Create a new screen that will show the role change information
ClosedPublic
Actions

Authored by rohan on Jun 9 2023, 11:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 8:04 AM
Unknown Object (File)
Sat, Dec 7, 2:14 AM
Unknown Object (File)
Sat, Dec 7, 2:14 AM
Unknown Object (File)
Sat, Dec 7, 2:14 AM
Unknown Object (File)
Sat, Dec 7, 2:14 AM
Unknown Object (File)
Sat, Dec 7, 2:14 AM
Unknown Object (File)
Fri, Dec 6, 6:21 PM
Unknown Object (File)
Thu, Dec 5, 1:37 AM
View All Files
Subscribers
ashoat
• ted
tomek

Details

Reviewers
atul
ginsu
Commits
rCOMMbb6d470e1792: [native] Create a new screen that will show the role change information
Summary

This is the screen that contains the information (user avatar, username, role change description and role change options) to allow admins to change roles for a specific user. The navigation and the
header left/right buttons will come in the subsequent diffs to make this one easier to review.

Depends on D8155

Test Plan

Please see below what the screen looks like for a given user

Simulator Screen Shot - iPhone 14 Pro - 2023-06-09 at 14.24.00.png (2×1 px, 143 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan created this revision.Jun 9 2023, 11:23 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 9 2023, 11:23 AM
rohan edited the test plan for this revision. (Show Details)Jun 9 2023, 11:24 AM
rohan attached a referenced file: F580165: Simulator Screen Shot - iPhone 14 Pro - 2023-06-09 at 14.24.00.png. (Show Details)
rohan added a child revision: D8157: [native] Navigate to the role change screen.Jun 9 2023, 11:26 AM
Harbormaster completed remote builds in B20135: Diff 27585.Jun 9 2023, 11:46 AM
rohan requested review of this revision.Jun 9 2023, 11:46 AM
atul requested changes to this revision.Jun 12 2023, 1:06 PM
atul added 1 blocking reviewer(s): ginsu.
atul added a subscriber: • ted.

Requesting changes for letterSpacing justification. Also adding @ginsu as blocking to take a look at showActionSheet(...), specifically how it behaves "across platforms."

native/roles/change-roles-screen.react.js
73 ↗(On Diff #27585)

It's weird that we need to grab activeTheme here, we should just be able to use the design system. But it looks like showActionSheetWithOptions(...) takes userInterfaceStyle, so I guess we have to?

82 ↗(On Diff #27585)

CC @ginsu

Is there anything iOS/Android specific we need to take into account here with indices? Or is that handled at a "lower" level of abstraction automatically for us?

102 ↗(On Diff #27585)

Wonder if we should use size="profile" for consistency w/ the other places we use avatar in settings screens? It looks like of sparse as is, and profile would "fill out the view" a bit better imo?

CC @ted

125 ↗(On Diff #27585)

Hm, why are we setting letterSpacing to 0.3 here? Are we sure that this was intentional? Messing w/ kerning is a pretty "extreme" decision, so wondering what the justification is

This revision now requires changes to proceed.Jun 12 2023, 1:06 PM
rohan added inline comments.Jun 12 2023, 2:11 PM
native/roles/change-roles-screen.react.js
73 ↗(On Diff #27585)

Yeah I thought so as well, but didn't see an option otherwise to specify userInterfaceStyle. I used invite-links-button.react.js as an action sheet reference

102 ↗(On Diff #27585)

Reference for what profile looks like for ted. If the consensus is to change here, I'll also change on web to keep it consistent

Simulator Screen Shot - iPhone 14 Pro - 2023-06-12 at 17.09.18.png (2×1 px, 189 KB)

125 ↗(On Diff #27585)

No you're right, this was unintentional

rohan attached a referenced file: F584774: Simulator Screen Shot - iPhone 14 Pro - 2023-06-12 at 17.09.18.png. (Show Details)Jun 12 2023, 2:11 PM
• ted added inline comments.Jun 12 2023, 2:32 PM
native/roles/change-roles-screen.react.js
102 ↗(On Diff #27585)

We can use size="profile" for consistency.

@rohan Can you check the username text styling? It looks a bit thin here in the screenshot

rohan added inline comments.Jun 13 2023, 4:52 AM
native/roles/change-roles-screen.react.js
102 ↗(On Diff #27585)

I forgot the fontWeight specified in the Figma! I'll add it now

rohan updated this revision to Diff 27681.Jun 13 2023, 4:53 AM

Remove letterSpacing, add fontWeight, set avatar size to profile

Harbormaster completed remote builds in B20206: Diff 27681.Jun 13 2023, 5:10 AM
ginsu requested changes to this revision.Jun 13 2023, 11:37 AM

Requesting changes for Android action/bottom sheet implementation. Here is the doc for android action/bottom sheet best practices

native/roles/change-roles-screen.react.js
82 ↗(On Diff #27585)

We should only make the cancel button show for iOS. We also should display icons in for android bottom sheet. I would recommend taking a look at useShowAvatarActionSheet in native/avatars/avatar-hooks for inspiration

This revision now requires changes to proceed.Jun 13 2023, 11:37 AM
rohan added inline comments.Jun 13 2023, 12:18 PM
native/roles/change-roles-screen.react.js
82 ↗(On Diff #27585)

Since the role names are not predefined and can be any variety of roles, I don't know if there's a practical way to use icons in the action sheet

rohan added a comment.EditedJun 13 2023, 12:19 PM
In D8156#242184, @ginsu wrote:

Requesting changes for Android action/bottom sheet implementation. Here is the doc for android action/bottom sheet best practices

Thanks for the doc, it's really helpful! I'll make the changes for the cancel button, just had one question about the icons above (accidentally submitted the comment too early)

Screenshot 2023-06-13 at 3.21.53 PM.png (1×1 px, 480 KB)

rohan attached a referenced file: F585851: Screenshot 2023-06-13 at 3.21.53 PM.png. (Show Details)Jun 13 2023, 12:22 PM
ginsu added inline comments.Jun 13 2023, 12:26 PM
native/roles/change-roles-screen.react.js
82 ↗(On Diff #27585)

Hmmm that's a good point... I think it might be fine to deviate from the "standard" practice here, but if I think of any other solutions, I'll let you know also gonna cc @ted if he has any ideas for this

rohan updated this revision to Diff 27700.Jun 13 2023, 12:27 PM

Address action sheet feedback

Harbormaster completed remote builds in B20222: Diff 27700.Jun 13 2023, 12:47 PM
atul accepted this revision.Jun 13 2023, 6:29 PM

Thanks for addressing feedback

Requesting changes for Android action/bottom sheet implementation. Here is the doc for android action/bottom sheet best practices

@ginsu, I think it could be good if we could "abstract away" these platform-specific quirks

ginsu accepted this revision.Jun 14 2023, 9:52 AM

@ginsu, I think it could be good if we could "abstract away" these platform-specific quirks

I agree, there is already task to create our own native bottomsheet/actionsheet component, abstracting away these platform specific quirks would be a good first step to this task

This revision is now accepted and ready to land.Jun 14 2023, 9:52 AM
rohan updated this revision to Diff 27946.Jun 21 2023, 8:06 AM

Rebase

This revision was landed with ongoing or failed builds.Jun 21 2023, 8:42 AM
Closed by commit rCOMMbb6d470e1792: [native] Create a new screen that will show the role change information (authored by rohan). · Explain Why
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMMbb6d470e1792: [native] Create a new screen that will show the role change information.
Harbormaster completed remote builds in B20392: Diff 27946.Jun 21 2023, 9:11 AM

Revision Contents
Changeset List

PathSize
native/
chat/
11 lines
navigation/
3 lines
roles/
177 lines

Diff 27965

View Options

native/chat/chat.react.js

Loading...
View Options

native/navigation/route-names.js

Loading...
View Options

native/roles/change-roles-screen.react.js

Loading...