-
Notifications
You must be signed in to change notification settings - Fork 373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [UIE-8140] - IAM RBAC - Assign New Roles drawer update #11834
base: develop
Are you sure you want to change the base?
feat: [UIE-8140] - IAM RBAC - Assign New Roles drawer update #11834
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodonnel-akamai I tried to explain things as thoroughly as I could this time around since you've asked. There may be some gaps in what I suggested, but read our docs and look at some other examples already in IAM. Happy to clarify any questions! 👍
const handleRemoveRole = (index: number) => { | ||
const updatedRoles = selectedRoles.filter((_, i) => i !== index); | ||
setSelectedRoles(updatedRoles); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to want to refactor this to use React Hook Form so a lot of this is going to change.
Essentially, you'll want to do something like:
const form = useForm<AssignNewRoleFormValues>({
defaultValues: {
roles: [{ role: null }],
},
});
// shared/utilities
export interface AssignNewRoleFormValues {
roles: {
role: RolesType | null;
}[];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing roles will be handled by remove
handler provided by RHF
const { control, handleSubmit, watch, reset } = form;
const { append, fields, remove } = useFieldArray({
control,
name: 'roles',
});
// We want to watch changes to this value since we're conditionally rendering "Add another role"
const roles = watch('roles');
selectedRoles.map((role, index) => ( | ||
<AssignSingleRole | ||
index={index} | ||
key={role ? role.label : `${index}`} | ||
onChange={handleChangeRole} | ||
onRemove={handleRemoveRole} | ||
options={allRoles} | ||
permissions={accountPermissions} | ||
selectedOption={selectedRoles[index]} | ||
/> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're getting the roles from our RHF fields, this will have to change:
fields.map((field, index) => (
<AssignSingleRole
index={index}
key={field.id}
onRemove={() => remove(index)}
options={allRoles}
permissions={accountPermissions}
selectedOption={field.role}
/>
))}
// eslint-disable-next-line no-console | ||
console.log( | ||
'Selected Roles:', | ||
selectedRoles.filter((role) => role) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use values from RHF
values.roles.map((r) => r.role).filter(Boolean)
{selectedRole && ( | ||
<AssignedPermissionsPanel key={selectedRole.name} role={selectedRole} /> | ||
{/* If all roles are filled, allow them to add another */} | ||
{selectedRoles.every((role) => role !== null) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the roles
that we're watching:
{roles.length > 0 && roles.every((field) => field.role) && (
<StyledLinkButtonBox | ||
sx={(theme) => ({ marginTop: theme.spacing(1.5) })} | ||
> | ||
<LinkButton onClick={addRole}>Add another role</LinkButton> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the built in append
:
<LinkButton onClick={() => append({ role: null })}>
export const AssignSingleRole = ({ | ||
options, | ||
index, | ||
selectedOption, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will no need for selectedOption
anymore with RHF
const selectedRole = React.useMemo(() => { | ||
if (!selectedOption || !permissions) { | ||
return null; | ||
} | ||
return getRoleByName(permissions, selectedOption.value); | ||
}, [selectedOption, permissions]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just inline this now that things are simpler:
<AssignedPermissionsPanel
role={getRoleByName(permissions, value.value)}
/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, we'll need to pass down the control
via context:
const { control } = useFormContext<AssignNewRoleFormValues>();
<Autocomplete | ||
renderOption={(props, option) => ( | ||
<li {...props} key={option.label}> | ||
{option.label} | ||
</li> | ||
)} | ||
label="Assign New Roles" | ||
options={options} | ||
value={selectedOption} | ||
onChange={(_, opt) => onChange(index, opt)} | ||
placeholder="Select a Role" | ||
textFieldProps={{ hideLabel: true }} | ||
/> | ||
{selectedRole && ( | ||
<AssignedPermissionsPanel | ||
key={selectedRole.name} | ||
role={selectedRole} | ||
/> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to wrap this in a Controller
from RHF so that it can update the parent fields:
<Controller
render={({ field: { onChange, value } }) => (
<>
<Autocomplete
onChange={(event, newValue) => {
onChange(newValue);
}}
renderOption={(props, option) => (
<li {...props} key={option.label}>
{option.label}
</li>
)}
label="Assign New Roles"
options={options}
placeholder="Select a Role"
textFieldProps={{ hideLabel: true }}
value={value || null}
/>
{value && (
<AssignedPermissionsPanel
role={getRoleByName(permissions, value.value)}
/>
)}
</>
)}
control={control}
name={`roles.${index}.role`}
/>
sx={(theme) => ({ | ||
flex: '0 1 auto', | ||
verticalAlign: 'top', | ||
marginTop: index === 0 ? theme.spacing(-0.5) : theme.spacing(2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with spacing tokens
marginTop: index === 0 ? theme.spacing(-0.5) : theme.spacing(2), | ||
})} | ||
> | ||
<Button disabled={index === 0} onClick={() => onRemove(index)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd petition UX to revisit having an X
button that's always disabled rather than just not showing it. Less code, cleaner, and better accessibility.
packages/manager/src/features/IAM/Users/UserRoles/AssignNewRoleDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/.changeset/pr-11834-upcoming-features-1741795011556.md
Outdated
Show resolved
Hide resolved
packages/manager/src/features/IAM/Users/UserRoles/AssignSingleRole.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have the ESLint extension on the IDE you are using?
display={'flex'} | ||
flexDirection={'column'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display={'flex'} | |
flexDirection={'column'} | |
display="flex" | |
flexDirection="column" |
<Divider | ||
sx={(theme) => ({ | ||
marginBottom: theme.spacing(1.5), | ||
})} | ||
></Divider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Divider | |
sx={(theme) => ({ | |
marginBottom: theme.spacing(1.5), | |
})} | |
></Divider> | |
<Divider | |
sx={(theme) => ({ | |
marginBottom: theme.spacing(1.5), | |
})} | |
/> |
76e4605
to
41bceb9
Compare
@jaalah-akamai thanks a lot for such a detailed explanation - it helped a lot! I’ve updated this PR according to your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaleksee-akamai This looks much better now. Thank you for addressing the feedback.
Coverage Report: ✅ |
41bceb9
to
ba507a1
Compare
ba507a1
to
9066886
Compare
@cpathipa , I've resolved conflicts |
Cloud Manager UI test results🎉 539 passing tests on test run #9 ↗︎
|
Description 📝
Adding in the functionality behind the Assign New Roles drawer for a single user
Changes 🔄
List any change(s) relevant to the reviewer.
Does not include (will be introduced in a future PR - TODO are listed in code):
Target release date 🗓️
(dev)
Preview 📷
Include a screenshot or screen recording of the change.
🔒 Use the Mask Sensitive Data setting for security.
💡 Use
<video src="" />
tag when including recordings in table.How to test 🧪
Prerequisites
(How to setup test environment)
Verification steps
(How to verify changes)
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅