refactor: style improvements for the User Management page #1131

andrewgolovanov · 2025-02-19T17:01:09Z

Description:
This pull request adds style improvements as well as refactoring of current components for the User Management page

Before:

After:

Before:

After:

Before:

After:

Before:

After:

Before:

After:

Before:

After:

CLAassistant · 2025-02-19T17:01:17Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

netlify · 2025-02-19T17:01:31Z

✅ Deploy Preview for harness-design ready!

Name	Link
🔨 Latest commit	`66d258f`
🔍 Latest deploy log	https://app.netlify.com/sites/harness-design/deploys/67b6106d4ff90d0008ff04b1
😎 Deploy Preview	https://deploy-preview-1131--harness-design.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-02-19T17:01:36Z

✅ Deploy Preview for harness-xd-review ready!

Name	Link
🔨 Latest commit	`66d258f`
🔍 Latest deploy log	https://app.netlify.com/sites/harness-xd-review/deploys/67b6106d8d249f00089fff69
😎 Deploy Preview	https://deploy-preview-1131--harness-xd-review.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

- Rename dialog hooks to use kebab-case filenames - Remove nested index files for dialog hooks - Simplify export structure for dialog hooks fix: standardize dialog content width and error text classes - Remove `w-full` class from Dialog.Content components - Reorder error text classes for consistency across user management dialogs fix: adjust user dialog UI details - Modify info icon positioning in create user dialog - Reorder error text classes for consistency - Use useEffect to reset form values in edit user dialog refactor: remove form components from user management dialogs - Inline dialog form logic directly into dialog components - Remove separate form components for create, edit, delete, reset password, and remove admin dialogs - Simplify dialog structure by consolidating form and dialog logic - Update locales to support new dialog translations fixes dialogs width decapitalize fix cancels buttons refactor: standardize import paths in user management components - Replace relative imports with absolute paths using @/ prefix - Maintain relative imports in index.ts files - Update import paths in dialog components and providers added locales fixes added empty state added error state added skeleton and nodata refactor dialogs added state provider caption props added no search results state paddings refactor remove unnecesary props drilling remove classes sorting refactor remove unnecessary props drilling fix-margins first versions of modals replace text with span change eslint rule due to conflict between prettier and eslint, prettier already sort classnames Adds promisifyMutation helper to simplify mutation handling Updates user management handlers to use the helper Adds proper typing for handlers using IDataHandlers interface Removes duplicate type definitions added search query make pagination using usePaginationQueryStateWithStore fixes code-review: remove onErrors callbacks fix enter fix pathes refactoring dialogs refactoring types commit: refactor: use store provider for user management hooks 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. commit: refactor: move dialog context to providers directory 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. remove redundant index from dialog commit: refactor: migrate dialog state management to UI layer 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. refactor: reorganize validation schemas and types - 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 refactor(user-management): reorganize dialogs structure - 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 first version of filters and sorting remove unnecessary fragment added tabs fixes after design review

knagurski · 2025-02-26T19:36:47Z

apps/gitness/src/pages-v2/user-management/user-management-container.tsx

-  const handleCreateUser = (data: { uid: string; email: string; display_name: string }) => {
-    createUser({
+  const handleCreateUser: IDataHandlers['handleCreateUser'] = data => {
+    return promisifyMutation(createUser, {


Instead of wrapping things in promisifyMutation, you can just the mutateAsync instead of mutate to get the promise-based version from Tanstack Query

https://tanstack.com/query/latest/docs/framework/react/guides/mutations#promises

knagurski · 2025-02-26T19:37:47Z

apps/gitness/src/pages-v2/user-management/utils/promisify-mutation.ts

I think we can kill this entirely and just use mutateAsync instead of mutate

knagurski · 2025-02-26T19:38:47Z

packages/ui/locales/en/views.json

+    "addNewUser": "Add a new user",
+    "userId": "User ID",
+    "userIdHint": "User ID cannot be changed once created",
+    "enterUsername": "Enter user name",


I'd argue that it's username, not user name. Or is this the user's name? In that case, we probably should just say "Enter name" to remove any ambiguity.

Suggested change

"enterUsername": "Enter user name",

"enterUsername": "Enter username",

Suggested change

"enterUsername": "Enter user name",

"enterName": "Enter name",

knagurski · 2025-02-26T19:54:53Z

packages/ui/src/views/user-management/components/dialogs/components/create-user/create-user.tsx

+            <Button variant="outline" type="button" onClick={onClose} disabled={isCreatingUser}>
+              {t('views:userManagement.cancel', 'Cancel')}
+            </Button>
+            <Button type="button" onClick={handleSubmit(onSubmit)} disabled={isCreatingUser}>


I don't know if the Dialog component allows this, but is it possible wrap the content in a form tag so that submission via keyboard would be possible? Instead of the submission being a click handler, it would then be set on the form and this button could then just be set as type="submit".

<Dialog.Content> <form onSubmit={handleSubmit(onSubmit)}> <Dialog.Header>...</Dialog.Header> {/* form stuff */} <Dialog.Footer> <ButtonGroup> <Button onclick={onClose}>Cancel</Button> <Button type="submit">Invite user</Button> </ButtonGroup> </Dialog.Footer> </form> </Dialog.Content>

Or something like that. Ideally most/all dialogs should be confirmable using enter and dismissible using escape without first having to focus those buttons.

knagurski · 2025-02-26T19:56:59Z

packages/ui/src/views/user-management/components/dialogs/components/create-user/schemas.ts

+  uid: z.string().min(1, { message: 'Please provide a user ID' }),
+  email: z.string().email({ message: 'Please enter a valid email address' }),
+  display_name: z.string().min(1, { message: 'Please provide a display name' })


Will these display? Or will they somehow be replaced by the translations?

knagurski · 2025-02-26T20:00:47Z

packages/ui/src/views/user-management/components/dialogs/components/create-user/types.ts

+export interface ICreateUserDialogProps {
+  handleCreateUser: (data: NewUserFields) => void
+  open: boolean
+  onClose: () => void
+}
+
+export interface ICreateUserFormProps {


I'm really not a fan of the I prefix on interfaces. For me, the Props suffix is indication enough that it is a type.

Suggested change

export interface ICreateUserDialogProps {

handleCreateUser: (data: NewUserFields) => void

open: boolean

onClose: () => void

}

export interface ICreateUserFormProps {

export interface CreateUserDialogProps {

handleCreateUser: (data: NewUserFields) => void

open: boolean

onClose: () => void

}

export interface CreateUserFormProps {

I also prefer when the component props interfaces live with their respective components. Easier to keep them in sync and see at a glance.

Neither are blockers since this is in an internal component of a view.

knagurski · 2025-02-26T20:28:50Z

packages/ui/src/views/user-management/components/dialogs/components/edit-user/edit-user.tsx

+  const onSubmit: SubmitHandler<EditUserFormType> = data => {
+    handleUpdateUser(data)
+  }


Do we need this intermediate function? Could we just use handleUpdateUser directly? i.e.

<Button type="button" onClick={handleSubmit(handleUpdateUser)} disabled={isUpdatingUser}>

knagurski · 2025-02-26T20:32:17Z

...ges/ui/src/views/user-management/components/dialogs/components/remove-admin/remove-admin.tsx

+          </Dialog.Title>
+        </Dialog.Header>
+
+        {/* TODO: investigate how to make user login bold with font-weight: 600 */}


Not quite sure what "user login" is referring to 🤔

knagurski · 2025-02-26T20:41:44Z

...ui/src/views/user-management/components/dialogs/components/reset-password/reset-password.tsx

+          </span>
+
+          {generatePassword && (
+            <Input


Just to confirm, the user who will be generating a password is unlikely to be the user who the password is for?

knagurski · 2025-02-26T20:47:11Z

packages/ui/src/views/user-management/components/dialogs/hooks/use-dialog-data.ts

+export const useDialogData = () => {
+  const { useAdminListUsersStore } = useUserManagementStore()
+
+  const { setUser, setPassword, setGeteneratePassword } = useAdminListUsersStore()


Looks like there's a typo in the store?

Suggested change

const { setUser, setPassword, setGeteneratePassword } = useAdminListUsersStore()

const { setUser, setPassword, setGeneratePassword } = useAdminListUsersStore()

knagurski · 2025-02-26T20:53:11Z

packages/ui/src/views/user-management/components/dialogs/hooks/use-dialog-data.ts

+  const prepareDialogData = (user: UsersProps | null, dialogType: DialogLabels) => {
+    if (user) setUser(user)
+
+    if (dialogType === DialogLabels.RESET_PASSWORD) {
+      setGeteneratePassword(false)
+      setPassword(generateAlphaNumericHash(10))
+
+      return
+    }
+
+    if (dialogType === DialogLabels.CREATE_USER) {
+      setGeteneratePassword(true)
+      setPassword(generateAlphaNumericHash(10))
+
+      return
+    }
+  }
+
+  const handleDialogOpen = (user: UsersProps | null, dialogType: DialogLabels) => {
+    prepareDialogData(user, dialogType)
+    openDialog(dialogType)
+  }
+
+  return {
+    handleDialogOpen
+  }


Worth pointing out that this will not be dependency array safe. Easiest way is to wrap this whole bit in a useMemo

Suggested change

const prepareDialogData = (user: UsersProps | null, dialogType: DialogLabels) => {

if (user) setUser(user)

if (dialogType === DialogLabels.RESET_PASSWORD) {

setGeteneratePassword(false)

setPassword(generateAlphaNumericHash(10))

return

}

if (dialogType === DialogLabels.CREATE_USER) {

setGeteneratePassword(true)

setPassword(generateAlphaNumericHash(10))

return

}

}

const handleDialogOpen = (user: UsersProps | null, dialogType: DialogLabels) => {

prepareDialogData(user, dialogType)

openDialog(dialogType)

}

return {

handleDialogOpen

}

return useMemo(() => {

const prepareDialogData = (user: UsersProps | null, dialogType: DialogLabels) => {

if (user) setUser(user)

if (dialogType === DialogLabels.RESET_PASSWORD) {

setGeteneratePassword(false)

setPassword(generateAlphaNumericHash(10))

return

}

if (dialogType === DialogLabels.CREATE_USER) {

setGeteneratePassword(true)

setPassword(generateAlphaNumericHash(10))

return

}

}

const handleDialogOpen = (user: UsersProps | null, dialogType: DialogLabels) => {

prepareDialogData(user, dialogType)

openDialog(dialogType)

}

return {

handleDialogOpen

}

}, [setUser, setPassword, setGeteneratePassword, openDialog])

knagurski · 2025-02-26T20:54:29Z

packages/ui/src/views/user-management/components/dialogs/hooks/use-dialog-handlers.ts

+export const useDialogHandlers = (handlers: IDialogHandlers) => {
+  const { closeDialog } = useDialogs()
+
+  return {


This also won't be dependency array safe. Easy fix with a useMemo around this whole return

knagurski · 2025-02-26T20:55:35Z

packages/ui/src/views/user-management/components/dialogs/hooks/use-dialog-handlers.ts

+
+  return {
+    handleCreateUser: async (...args: Parameters<typeof handlers.handleCreateUser>) => {
+      const result = await handlers.handleCreateUser(...args)


What happens if these handlers fail?

knagurski · 2025-02-26T20:57:36Z

packages/ui/src/views/user-management/components/main.tsx

+  const { totalPages, page: currentPage, setPage } = useAdminListUsersStore()
+
+  return (
+    <SandboxLayout.Main className="max-w-[1092px]">


1092 is a very peculiarly specific number 🤔

andrewgolovanov requested a review from knagurski as a code owner February 19, 2025 17:01

andrewgolovanov mentioned this pull request Feb 19, 2025

Review User Management #713

Open

andrewgolovanov force-pushed the users-management branch from d1c7a50 to 66d258f Compare February 19, 2025 17:10

knagurski reviewed Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: style improvements for the User Management page #1131

refactor: style improvements for the User Management page #1131

andrewgolovanov commented Feb 19, 2025

CLAassistant commented Feb 19, 2025

netlify bot commented Feb 19, 2025 •

edited

Loading

netlify bot commented Feb 19, 2025 •

edited

Loading

knagurski Feb 26, 2025

knagurski Feb 26, 2025

knagurski Feb 26, 2025 •

edited

Loading

knagurski Feb 26, 2025

knagurski Feb 26, 2025

knagurski Feb 26, 2025 •

edited

Loading

knagurski Feb 26, 2025

knagurski Feb 26, 2025

knagurski Feb 26, 2025

knagurski Feb 26, 2025

knagurski Feb 26, 2025

knagurski Feb 26, 2025

knagurski Feb 26, 2025

knagurski Feb 26, 2025

	"enterUsername": "Enter user name",
	"enterUsername": "Enter username",

	"enterUsername": "Enter user name",
	"enterName": "Enter name",

	const { setUser, setPassword, setGeteneratePassword } = useAdminListUsersStore()
	const { setUser, setPassword, setGeneratePassword } = useAdminListUsersStore()

refactor: style improvements for the User Management page #1131

Are you sure you want to change the base?

refactor: style improvements for the User Management page #1131

Conversation

andrewgolovanov commented Feb 19, 2025

CLAassistant commented Feb 19, 2025

netlify bot commented Feb 19, 2025 • edited Loading

✅ Deploy Preview for harness-design ready!

netlify bot commented Feb 19, 2025 • edited Loading

✅ Deploy Preview for harness-xd-review ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagurski Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagurski Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Feb 19, 2025 •

edited

Loading

netlify bot commented Feb 19, 2025 •

edited

Loading

knagurski Feb 26, 2025 •

edited

Loading

knagurski Feb 26, 2025 •

edited

Loading