-
Notifications
You must be signed in to change notification settings - Fork 367
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-8138] - IAM RBAC: add new assigned roles table component (part 1) #11533
feat: [UIE-8138] - IAM RBAC: add new assigned roles table component (part 1) #11533
Conversation
73028d5
to
bd4e6aa
Compare
@@ -94,7 +94,7 @@ export const UserDetailsLanding = () => { | |||
<UserDetails /> | |||
</SafeTabPanel> | |||
<SafeTabPanel index={++idx}> | |||
<UserRoles assignedRoles={assignedRoles} /> | |||
<UserRoles /> |
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.
@bnussman-akamai , I've decided to move it to the component as you previously suggested. I'll do the same for UserEntities
later
@@ -9,7 +9,7 @@ import { CollapsibleRow } from './CollapsibleRow'; | |||
export interface TableItem { | |||
InnerTable: JSX.Element; | |||
OuterTableCells: JSX.Element; | |||
id: number; | |||
id: number | string; |
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 don’t have any data that can be used as an ID of type number. Only role names are unique, so we need to add a string type here
Coverage Report: ✅ |
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.
Overall, the component looks good! and well structured. I've left a few comments to improve the AssignedRolesTable
. Given its size, we could move some util functions to a utilities.ts file to keep the component more focused and readable. Additionally, I recommend adding explicit return types and memoization where needed for better type safety.
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Outdated
Show resolved
Hide resolved
bd4e6aa
to
74f2dc6
Compare
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Show resolved
Hide resolved
packages/manager/src/features/IAM/Shared/AssignedRolesTable/AssignedRolesTable.tsx
Show resolved
Hide resolved
Cloud Manager UI test results🔺 2 failing tests on test run #3 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: yarn cy:run -s "cypress/e2e/core/cloudpulse/cloudpulse-navigation.spec.ts,cypress/e2e/core/linodes/smoke-linode-landing-table.spec.ts" |
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 Thank you for addressing all the feedback. LGTM!
Description 📝
IAM RBAC - new assigned roles table component.
figma
Changes 🔄
List any change(s) relevant to the reviewer.
do not include (will be introduced in the next PR):
Target release date 🗓️
1/28/25 (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 ✅