Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new "Diff Users" page to the application, including corresponding navigation, API methods, and routing updates.
- Added a new navigation link in NavItems for "Diff Users".
- Introduced new API methods (fetchGetAllUsers and useGetAllUsers) and types to support fetching all users data.
- Updated App.tsx to include a new route for the DiffUsers page.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/components/NavItems.tsx | Added a navigation link for the "Diff Users" page with a Divider. |
| src/api/apiComponents.ts | Added new API functions and types for fetching all users. |
| src/App.tsx | Updated routing to include the DiffUsers page. |
Comments suppressed due to low confidence (1)
src/components/NavItems.tsx:169
- It appears that GroupIcon is used but not imported. Please ensure that GroupIcon is properly imported from its module.
<ListItemLink to="/users/diff" displayText="Diff Users" displayIcon={<GroupIcon />} />
| const groupTypeLabels = { | ||
| role_group: 'Role Group', | ||
| app_group: 'App Group', | ||
| okta_group: 'Okta Group', |
There was a problem hiding this comment.
Technically they all groups (groups, app groups, roles) are okta groups and we don't specifically call them okta groups anywhere else in the app so probably better to change this to just 'Group'. Also we made Access to be compatible with other idp providers besides Okta so users at other companies might get confused
| return ( | ||
| <Box> | ||
| <Typography variant="h4" component="h1" gutterBottom> | ||
| User Access Comparison |
There was a problem hiding this comment.
Can you make a title card to match the style of other pages? We don't have any other pages with a title directly on the background layer like that
| {groupName ?? 'Unnamed Group'} | ||
| </Link> | ||
| </Typography> | ||
| <Chip |
There was a problem hiding this comment.
Idk why this is showing up as pink but it'd probably be better to stick to the theme colors? Also filled chips would probably be more readable
| </Collapse> | ||
| <Divider /> | ||
|
|
||
| <ListItemLink to="/users/diff" displayText="Diff Users" displayIcon={<GroupIcon />} /> |
There was a problem hiding this comment.
I lean towards having this be linked from the users page as opposed to being in the navbar.
That said, all the other items in the nav bar have unique icons. If this stays here as opposed to being moved, probably change the icon to be consistent with everything else
| </Typography> | ||
|
|
||
| <Box display="flex" justifyContent="space-between" alignItems="center" mb={3}> | ||
| <Button variant="contained" size="large" sx={{height: '50px'}} onClick={resetComparison}> |
There was a problem hiding this comment.
A filled button would probably be more readable here
2 participants
Uh oh!
There was an error while loading. Please reload this page.