-
Notifications
You must be signed in to change notification settings - Fork 879
feat: add custom roles #14069
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: add custom roles #14069
Conversation
🤖 Meticulous replayed 100 user sessions and took 1258 visual snapshots. Meticulous has not yet run on 238e995 of the main branch and so there was nothing to compare against. Last updated for commit 72f1bab. This comment will update as new commits are pushed. |
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.
couple smalls things that could be cleaned up before merging, but overall looks great! 😄
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/CustomRolesPage/CustomRolesPage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePageView.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePageView.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.
my main concern is the assign_org_role
check in AuthProvider
. I think we should remove that and just use the check in the organizationPermissions
query everywhere. I tagged Steven so he can give a second opinion, because I'm not super familiar with the permissions checks stuff.
other than that, just some minor things! looking good :)
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePageView.tsx
Show resolved
Hide resolved
: filteredRBACResourceActions; | ||
|
||
return ( | ||
<> |
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.
unnecessary fragment?
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.
marking as unresolved again because I still see this in the code :p
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePageView.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePageView.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/CustomRolesPage/CustomRolesPageView.tsx
Outdated
Show resolved
Hide resolved
I will rely on @aslilac review for the FE code and focus on the design and QA.
|
This comment was marked as resolved.
This comment was marked as resolved.
@BrunoQuaresma Regarding using switches for organization roles similar to notifications.
|
@BrunoQuaresma regarding the form only having one section, I still need to add the delete role functionality and was thinking of doing it like organization settings. Unless, you have any thoughts about a better place to delete roles? ![]() |
Related to the checkboxes: I think using GitHub as a reference is a good idea, but I missed a few things like the alignment of the descriptions and the "header checkbox" to check or uncheck the children options. Related to the delete section: I don't think we need it on the create page right? Also, the width page for those settings pages is smaller because the sidebar does not give proper space to a horizontal form. Even using two sections, we can still use vertical forms. GH does this very well on their settings pages. I will leave those decisions up to you. |
While QAing it I recorded a video to share a few design things that could be improved. Screen.Recording.2024-08-08.at.10.20.25.mp4 |
@aslilac I made all the changes planned for this PR based on the suggestions. Design improvements for checkboxes and delete custom role will be completed in a separate PR. |
align="right" | ||
sx={{ paddingTop: 0.4, paddingBottom: 0.4 }} | ||
> | ||
<FormControlLabel |
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 love the addition of this! could we add something similar to the bottom of the table too, just to make sure people can find it? a small extra row that just says "show all permissions" like this does
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 added the show all permissions checkbox in a table footer row
: filteredRBACResourceActions; | ||
|
||
return ( | ||
<> |
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.
marking as unresolved again because I still see this in the code :p
Resolve #13911