Details

Reviewers

atul
ginsu

Commits

rCOMMf3684a725e87: [web] Create the modals that will allow the user to confirm role deletion

Summary

This should be the final stack in the entire roles project. This is the client-side logic for web to handle role deletion. Per the designs, the warning message is the same as on native, with
either a generic deletion message if nobody is assigned the role or a more specific one if members have this custom role.

ENG-4432

Depends on D8624

Test Plan

Verified that the modal appears as expected for roles with and without members, and pressing the delete button actually deletes

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Jul 25 2023, 8:23 AM

Herald added a subscriber: tomek. · View Herald TranscriptJul 25 2023, 8:23 AM

rohan edited the test plan for this revision. (Show Details)Jul 25 2023, 8:24 AM

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

rohan edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B21189: Diff 29024.Jul 25 2023, 8:44 AM

rohan requested review of this revision.Jul 25 2023, 8:44 AM

rohan added inline comments.Jul 25 2023, 9:25 AM

web/roles/delete-role-modal.react.js
46–50 ↗	(On Diff #29024)	cc @ashoat for the copy (same as in D8574), the copy will be more visible in the test plan video

Only thing blocking this diff is awaiting the call to callDeleteCommunityRole(...) before calling popModal(...).

web/roles/delete-role-modal.react.js
39–50 ↗	(On Diff #29024)	We could create a function above the component that takes in `memberCount` and `defaultRoleName` and returns a string to reduce clutter here? Totally up to you
52–68 ↗	(On Diff #29024)	Same feedback as in D8595 where we should `await` the call to `callDeleteCommunityRole(...)` before calling `popModal(...)`
web/roles/role-actions-menu.react.js
31 ↗	(On Diff #29024)	Isn't this `invariant` really more "default role should exist"?
35 ↗	(On Diff #29024)	Isn't this invariant really more "existing role should exist"?