Page MenuHomePhabricator

[web] Add save functionality to the create role modal
ClosedPublic

Authored by rohan on Jul 21 2023, 9:16 AM.
Tags
None
Referenced Files
F2064772: D8595.id28945.diff
Fri, Jun 21, 8:10 AM
Unknown Object (File)
Wed, Jun 19, 11:15 PM
Unknown Object (File)
Wed, Jun 19, 3:30 PM
Unknown Object (File)
Wed, Jun 19, 12:03 PM
Unknown Object (File)
Sun, Jun 16, 12:15 PM
Unknown Object (File)
Fri, Jun 14, 10:23 PM
Unknown Object (File)
Thu, Jun 13, 3:35 PM
Unknown Object (File)
Wed, Jun 12, 7:45 AM
Subscribers

Details

Summary

This adds functionality to the Create button in the create-role modal, where clicking it will save the role and close the modal. Error handling for a duplicate role name will come later in the stack

ENG-4434

Depends on D8594

Test Plan

Confirmed that a new role is created when the button is clicked

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan requested review of this revision.Jul 21 2023, 9:37 AM
atul requested changes to this revision.Jul 26 2023, 11:29 AM

We should make sure that the request is complete before dismissing the modal.

web/roles/create-roles-modal.react.js
118–130 ↗(On Diff #28945)

Should we wait for callModifyCommunityRole to complete successfully before dismissing modal? It looks pretty instantaneous in local dev environment, but might take a second on prod.

We can pass an IIFE as the second argument to dispatchActionPromise that wraps the call to callModifyCommunityRole and calls popModal() after awaiting callModifyCommunityRole?

Here's an example of passing in IIFE that wraps call* to dispatchActionPromise:

8d8fe5.png (968×1 px, 168 KB)

(lib/components/base-edit-thread-avatar-provider.react.js)

This will also let us catch error which would be helpful for "TODO: Error handling in a later diff"?

Here's an example:

3d28ef.png (842×1 px, 170 KB)

(native/community-creation/community-configuration.react.js)

This revision now requires changes to proceed.Jul 26 2023, 11:29 AM

Address feedback (added a eslint suppression since otherwise I got a Unnecessary try/catch wrapper error. This should be addressed once I display an unknown error alert to the client). Also added a loading indicator

keyserver/src/creators/role-creator.js
117 ↗(On Diff #29069)

Oh forgot to remove this

atul requested changes to this revision.Jul 26 2023, 12:01 PM

Passing back to remove setTimeout

This revision now requires changes to proceed.Jul 26 2023, 12:01 PM
atul added inline comments.
web/roles/create-roles-modal.react.js
145–147 ↗(On Diff #29071)

Instead of catching and re-throwing, can we remove the try/catch altogether for now?

161–169 ↗(On Diff #29071)

Was going to suggest something like

const saveButtonContent = React.useMemo(
  () =>
    createRolesLoadingStatus === 'loading' ? (
      <LoadingIndicator status={createRolesLoadingStatus} size="medium" />
    ) : (
      'Save'
    ),
  [createRolesLoadingStatus],
);

but it ends up being the same number of lines after being formatted lol (and arguably less readable?)

This revision is now accepted and ready to land.Jul 26 2023, 12:25 PM

Remove try/catch in this diff

Update callsite following D8564 where I change the types