Page MenuHomePhabricator

[native] Create a ChangeRolesHeaderRightButton to save role changes
ClosedPublic

Authored by rohan on Jun 9 2023, 11:42 AM.
Tags
None
Referenced Files
F3349743: D8160.id27950.diff
Fri, Nov 22, 6:54 PM
F3349456: D8160.id27970.diff
Fri, Nov 22, 6:03 PM
Unknown Object (File)
Sun, Nov 17, 4:34 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, 1:20 PM
Unknown Object (File)
Sat, Nov 9, 11:32 AM
Unknown Object (File)
Sat, Nov 9, 11:31 AM
Subscribers

Details

Summary

This diff creates the 'save' headerRight component that is used to actually process the role change. When save is clicked, one of two things happen:

(1) If a member's role is changed, change the role and close the screen

(2) If a member's role remains unchanged, just close the screen

Because the save button is located in the react navigation header component, the best way I found to handle keeping track of the data from the role change screen was to access the info from route.params
after the role change screen sets them via navigation.setParams. If there is a better way to do this though, I'm open to changing this approach. I used this github issue as a reference for this approach.

Depends on D8159

Test Plan

Please see the video below showing changing a member's role

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan requested review of this revision.Jun 9 2023, 11:58 AM
native/roles/change-roles-header-right-button.react.js
49–60 ↗(On Diff #27589)

No real need to declare an additional function here. You can keep the promise from calling callChangeThreadMemberRoles as a separate variable if you'd like for readability, but that doesn't require declaring a new function

(Shellcheck CI job should pass after pulling in latest changes.)

atul requested changes to this revision.Jun 12 2023, 1:20 PM

Can we show a spinner when the request is in progress? It's probably pretty "instant" in your dev environment, but could look janky on prod if things are frozen until the request completes.

Did something pretty similar recently in CommunityCreationMembers that you could probably follow closely for inspiration:

cc6d47.png (1×1 px, 221 KB)

native/roles/change-roles-header-right-button.react.js
34 ↗(On Diff #27641)

We don't usually destructure values from useColors, maybe because we often use quite a few... but don't see anything wrong w/ this.

This revision now requires changes to proceed.Jun 12 2023, 1:20 PM

Rebase for CI and add a loading indicator (involved moving the headerRight declaration from chat.react.js to inside the change roles screen )

This revision is now accepted and ready to land.Jun 13 2023, 6:53 PM
This revision was landed with ongoing or failed builds.Jun 21 2023, 8:42 AM
This revision was automatically updated to reflect the committed changes.
native/roles/change-roles-screen.react.js
48

Memos with no dependencies generally don't need to be memos, and should instead be declared one time at the top-level scope

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

Or activityIndicatorStyle should be defined in unboundStyles?

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

Ah yeah, that makes more sense