-
Notifications
You must be signed in to change notification settings - Fork 0
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
Users management review #18
Conversation
…arness#834) Signed-off-by: Calvin Lee <[email protected]> Co-authored-by: Calvin Lee <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
* chore: rebase * chore: rebase * chore: fix hr scroll on pr list page * chore: fix height on pr details * fix: cursor, lint
* feat: add repo empty view * feat: add preview * fix: minor design fixes * fix: spacing
- Move all dialogs to dedicated folders with proper structure - Add types and index files for each dialog - Update imports and exports - Add new translations for empty state - Improve code organization in UserManagementPage
- Move Zod schemas to dedicated schema files - Update type definitions to use schema inference - Fix import paths to use absolute imports - Remove duplicate schema definitions from components - Centralize types in dedicated type files
The commit refactors the user management dialog state handling by: Moving dialog state management from container to UI layer using DialogsProvider Converting mutation handlers to return Promises for proper async handling Removing redundant dialog state management code from container Consolidating error and loading states into dedicated objects Moving EActiveTab enum to UI package for better organization This change improves separation of concerns by keeping UI state management in the UI layer while maintaining the same functionality.
This commit reorganizes the dialog-related code by: Moving DialogsProvider from context/ to providers/ directory Updating import paths across components to use the new location 3. Removing unused context files and types Consolidating dialog-related code for better maintainability This is a structural change that improves code organization while maintaining existing functionality.
This commit refactors the user management page to use a centralized store provider by: Creating UserManagementStoreProvider to manage shared hooks Removing hook props drilling through components Accessing hooks via useUserManagementStore hook in child components Splitting UserManagementPage into container and content components This change improves code organization and reduces prop drilling while maintaining existing functionality.
@@ -41,5 +43,15 @@ export const useAdminListUsersStore = create<IAdminListUsersStore>(set => ({ | |||
set({ | |||
generatePassword | |||
}) | |||
}, | |||
setSearchQuery: (searchQuery: 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 need to keep search in AdminListUsersStore
searchQuery | ||
}) | ||
}, | ||
setActiveTab: (activeTab: EActiveTab) => { |
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 need to keep tab in AdminListUsersStore
|
||
import { parseAsInteger, useQueryState } from '../../framework/hooks/useQueryState' | ||
import { useTranslationStore } from '../../i18n/stores/i18n-store' | ||
import { generateAlphaNumericHash } from '../pull-request/pull-request-utils' | ||
import { useAdminListUsersStore } from './stores/admin-list-store' | ||
|
||
export const UserManagementPageContainer = () => { | ||
const [queryPage, setQueryPage] = useQueryState('page', parseAsInteger.withDefault(1)) |
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.
for queryPage let's use usePaginationQueryStateWithStore
{}, | ||
{ | ||
onSuccess: () => { | ||
setDeleteUserDialogOpen(false) | ||
queryClient.invalidateQueries({ queryKey: ['adminListUsers'] }) | ||
}, | ||
onError: error => { |
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.
if u use error from query hook, let's remove onError callback
@@ -142,7 +105,7 @@ export const UserManagementPageContainer = () => { | |||
) | |||
|
|||
const handleCreateUser = (data: { uid: string; email: string; display_name: 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.
Let's create separate types for data in the UI so that we don't have to define them repeatedly in different places. This way, both in component props and here, we can simply specify the type.
/> | ||
<Spacer size={5} /> | ||
<UsersList users={filteredUsers} handleDialogOpen={handleDialogOpen} /> | ||
<Spacer size={8} /> |
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.
don't need this spacer, already have in PaginationComponent
|
||
return ( | ||
<> | ||
<Text size={5} weight={'medium'}> |
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.
Let's avoid using the Text component and replace it with h1 or other appropriate tags
PR Changes Summary
1. Store Updates (admin-list-store.tsx)
searchQuery
,activeTab
setSearchQuery
,setActiveTab
EActiveTab
enum integration2. Dialog Management Refactor
DialogsProvider
with context and hooksuseDialogHandlers
for async mutation handling3. Component Organization
4. Type System Improvements
5. Localization Updates
6. UI/UX Improvements
7. Container Updates (user-management-container.tsx)
8. Code Structure