[permissions][FE] followup design fixes 4 #12737
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Major UI/UX improvements to the role permissions interface focused on clarity and usability.
- Changed crucial descriptions for better clarity: 'Actions you can perform on all objects' → 'Actions users can perform on all objects', and similar changes throughout permission sections
- Introduced distinct permission tracking with new 'grantedBy' field to show 'Granted on X objects' status alongside existing 'Revoked on X objects'
- Fixed component instance IDs by appending roleId to prevent tab state sharing between different roles
- Added comprehensive select-all functionality for settings permissions with proper state management
- Made UI more robust by adding proper null checks and fallbacks, like defaulting to IconUserPin when role icon is undefined
17 files reviewed, 7 comments
Edit PR Review Bot Settings | Greptile
| <TabList | ||
| tabs={tabs} | ||
| className="tab-list" | ||
| componentInstanceId={SETTINGS_ROLE_DETAIL_TABS.COMPONENT_INSTANCE_ID} | ||
| componentInstanceId={ | ||
| SETTINGS_ROLE_DETAIL_TABS.COMPONENT_INSTANCE_ID + '-' + roleId | ||
| } | ||
| /> |
There was a problem hiding this comment.
style: Extract COMPONENT_INSTANCE_ID + '-' + roleId into a constant or memoized value to avoid string concatenation in render
...s/role-permissions/objects-permissions/components/SettingsRolePermissionsObjectsTableRow.tsx
Outdated
Show resolved
Hide resolved
.../roles/role-permissions/objects-permissions/types/SettingsRolePermissionsObjectPermission.ts
Show resolved
Hide resolved
| grantedBy: | ||
| objectPermissions?.filter( | ||
| (permission) => | ||
| permission.canReadObjectRecords === true && | ||
| settingsDraftRole.canReadAllObjectRecords === false, | ||
| )?.length ?? 0, | ||
| revokedBy: |
There was a problem hiding this comment.
style: Consider extracting the permission filter logic into a utility function since it's repeated for each permission type
...sions/object-level-permissions/components/SettingsRolePermissionsObjectLevelObjectPicker.tsx
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/roles/components/SettingsRolesDefaultRole.tsx
Outdated
Show resolved
Hide resolved
...e-permissions/settings-permissions/components/SettingsRolePermissionsSettingsTableHeader.tsx
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:25972 This environment will automatically shut down when the PR is closed or after 5 hours. |
| @@ -19,7 +19,7 @@ export const SettingsRolesContainer = () => { | |||
| const settingsAllRoles = useRecoilValue(settingsAllRolesSelector); | |||
| const settingsRolesIsLoading = useRecoilValue(settingsRolesIsLoadingState); | |||
|
|
|||
| if (settingsRolesIsLoading) { | |||
| if (settingsRolesIsLoading && !settingsAllRoles) { | |||
There was a problem hiding this comment.
Nit but im a bit confused by the settings prefix here. what is settingsAllRoles ? roles ?
There was a problem hiding this comment.
Ah, that's because every component in the setting module seemed to be prefixed with Settings and I did the same with states. Now I guess it's a bit confusing because we have settings tab in our roles so we have SettingsRoleSettings or even settings permissions (which is probably the most confusing one)
| anchorSelect={`#${containerId}`} | ||
| content={ | ||
| permissionValue === false | ||
| ? t`${roleLabel} can't ${humanReadableAction} ${objectLabel} records` |
There was a problem hiding this comment.
im curious about how translations work with interpolations
There was a problem hiding this comment.
it should work fine, we already have it in other places I believe 🤔 (but to double check indeed)
| canDestroyObjectRecords: t`destroy`, | ||
| }; | ||
|
|
||
| return permissionAction[objectPermissionKey] ?? t`see`; |
There was a problem hiding this comment.
No you are right, this is unnecessary as it should never happen, I'll change that
| ); | ||
|
|
||
| return ( | ||
| <TableRow gridAutoColumns="3fr 4fr 24px"> |
There was a problem hiding this comment.
does this work with every screen size ?
There was a problem hiding this comment.
It should yes, I tested on different sizes with smaller ones and seems fine
## Context - Whole row is now clickable - Fix padding on role tables - Fix tab being persistant between roles - Change various texts/descriptions - Add un/check all on settings permissions - Fix flash between role detail and roles - Add "Granted for X object(s)" - Swap permissions and assignment tabs position - add tooltip for object level permission actions - Add the inherited info on object-level permissions
Context