- Notifications
You must be signed in to change notification settings - Fork926
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat: add custom roles#14069
Uh oh!
There was an error while loading.Please reload this page.
Conversation
alwaysmeticulousbot commentedJul 31, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
🤖 Meticulous replayed 100 user sessions andtook 1258 visual snapshots. Meticulous has not yet run on238e995 of the main branch and so there was nothing to compare against. Last updated for commit72f1bab. 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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/CustomRolesPage/CustomRolesPage.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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 theassign_org_role
check inAuthProvider
. I think we should remove that and just use the check in theorganizationPermissions
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.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
: 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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/CustomRolesPage/CreateEditRolePageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/ManagementSettingsPage/CustomRolesPage/CustomRolesPageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
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
2e05329
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Resolve#13911