-
Notifications
You must be signed in to change notification settings - Fork 5
feat!: migrate Employee.Profile to hook architecture and add steady-state edit mode #1674
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
base: main
Are you sure you want to change the base?
Changes from all commits
6fb01af
4ae9aab
2928681
4325a45
33a689b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| .container { | ||
| width: 100%; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| import { useState } from 'react' | ||
| import { useTranslation } from 'react-i18next' | ||
| import classNames from 'classnames' | ||
| import { useEmployeeDetailsForm } from '../shared/useEmployeeDetailsForm' | ||
| import styles from './Profile.module.scss' | ||
| import { | ||
| BaseBoundaries, | ||
| BaseLayout, | ||
| type BaseComponentInterface, | ||
| type CommonComponentInterface, | ||
| } from '@/components/Base' | ||
| import { ActionsLayout } from '@/components/Common' | ||
| import { Form } from '@/components/Common/Form' | ||
| import { Grid } from '@/components/Common/Grid/Grid' | ||
| import { SDKFormProvider } from '@/partner-hook-utils/form/SDKFormProvider' | ||
| import { useI18n, useComponentDictionary } from '@/i18n' | ||
| import { componentEvents } from '@/shared/constants' | ||
| import { useComponentContext } from '@/contexts/ComponentAdapter/useComponentContext' | ||
|
|
||
| export interface ProfileProps extends CommonComponentInterface<'Employee.Profile'> { | ||
| employeeId: string | ||
| onEvent: BaseComponentInterface['onEvent'] | ||
| } | ||
|
|
||
| export function Profile({ | ||
| FallbackComponent, | ||
| ...props | ||
| }: ProfileProps & Pick<BaseComponentInterface, 'FallbackComponent'>) { | ||
| return ( | ||
| <BaseBoundaries componentName="Employee.Profile" FallbackComponent={FallbackComponent}> | ||
| <ProfileRoot {...props} /> | ||
| </BaseBoundaries> | ||
| ) | ||
| } | ||
|
|
||
| function ProfileRoot({ employeeId, className, dictionary, onEvent }: ProfileProps) { | ||
| useI18n('Employee.Profile') | ||
| useComponentDictionary('Employee.Profile', dictionary) | ||
| const { t } = useTranslation('Employee.Profile') | ||
| const Components = useComponentContext() | ||
|
|
||
| const employeeDetails = useEmployeeDetailsForm({ | ||
| employeeId, | ||
| withSelfOnboardingField: false, | ||
| optionalFieldsToRequire: { | ||
| update: ['firstName', 'lastName', 'email', 'dateOfBirth', 'ssn'], | ||
| }, | ||
| }) | ||
|
dmortal marked this conversation as resolved.
|
||
|
|
||
| const [showSuccess, setShowSuccess] = useState(false) | ||
|
|
||
| if (employeeDetails.isLoading) { | ||
| return <BaseLayout isLoading error={employeeDetails.errorHandling.errors} /> | ||
| } | ||
|
|
||
| const Fields = employeeDetails.form.Fields | ||
|
|
||
| const handleSubmit = async () => { | ||
| setShowSuccess(false) | ||
| const result = await employeeDetails.actions.onSubmit({ | ||
| onEmployeeUpdated: emp => { | ||
| onEvent(componentEvents.EMPLOYEE_UPDATED, emp) | ||
| }, | ||
| }) | ||
| if (!result) return | ||
| setShowSuccess(true) | ||
| } | ||
|
|
||
| const handleCancel = () => { | ||
| onEvent(componentEvents.CANCEL) | ||
| } | ||
|
|
||
| const alert = showSuccess ? ( | ||
| <Components.Alert | ||
| status="success" | ||
| label={t('successAlert')} | ||
| onDismiss={() => { | ||
| setShowSuccess(false) | ||
| }} | ||
| /> | ||
| ) : undefined | ||
|
|
||
| return ( | ||
| <section className={classNames(styles.container, className)}> | ||
| <BaseLayout error={employeeDetails.errorHandling.errors}> | ||
| <SDKFormProvider formHookResult={employeeDetails}> | ||
| <Form onSubmit={handleSubmit}> | ||
| {alert} | ||
| <Components.Heading as="h1">{t('title')}</Components.Heading> | ||
| <Grid | ||
| gap={{ base: 20, small: 8 }} | ||
| gridTemplateColumns={{ base: '1fr', small: ['1fr', 200] }} | ||
| > | ||
| <Fields.FirstName | ||
| label={t('firstName')} | ||
| validationMessages={{ | ||
| REQUIRED: t('validations.firstName'), | ||
| INVALID_NAME: t('validations.firstName'), | ||
| }} | ||
| /> | ||
| <Fields.MiddleInitial label={t('middleInitial')} /> | ||
| </Grid> | ||
| <Fields.LastName | ||
| label={t('lastName')} | ||
| validationMessages={{ | ||
| REQUIRED: t('validations.lastName'), | ||
| INVALID_NAME: t('validations.lastName'), | ||
| }} | ||
| /> | ||
| <Fields.Email | ||
| label={t('email')} | ||
| description={t('emailDescription')} | ||
| validationMessages={{ | ||
| REQUIRED: t('validations.email'), | ||
| INVALID_EMAIL: t('validations.email'), | ||
| EMAIL_REQUIRED_FOR_SELF_ONBOARDING: t('validations.email'), | ||
| }} | ||
| /> | ||
| <Fields.Ssn | ||
| label={t('ssnLabel')} | ||
| validationMessages={{ | ||
| INVALID_SSN: t('validations.ssn', { ns: 'common' }), | ||
| REQUIRED: t('validations.ssnRequired', { ns: 'common' }), | ||
| }} | ||
| /> | ||
| <Fields.DateOfBirth | ||
| label={t('dobLabel')} | ||
| validationMessages={{ REQUIRED: t('validations.dob', { ns: 'common' }) }} | ||
| /> | ||
| <ActionsLayout> | ||
| <Components.Button variant="secondary" onClick={handleCancel}> | ||
| {t('cancelCta')} | ||
| </Components.Button> | ||
| <Components.Button type="submit" isLoading={employeeDetails.status.isPending}> | ||
| {t('saveCta')} | ||
| </Components.Button> | ||
| </ActionsLayout> | ||
| </Form> | ||
| </SDKFormProvider> | ||
| </BaseLayout> | ||
| </section> | ||
| ) | ||
| } | ||
|
Comment on lines
+1
to
+143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Major | New management Profile component (143 lines) has no test file. The analogous FederalTaxes/management/FederalTaxes.test.tsx covers rendering, form validation, cancel/save flows, and event emissions (CANCEL, EMPLOYEE_FEDERAL_TAXES_UPDATED). This component has similar testable behaviors: form rendering, handleSubmit with success alert, handleCancel emitting CANCEL event, loading state, and error handling — all untested. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,16 +46,18 @@ export interface EmployeeDetailsSubmitCallbacks { | |
| onOnboardingStatusUpdated?: (status: unknown) => void | ||
| } | ||
|
|
||
| export interface UseEmployeeDetailsFormProps { | ||
| companyId: string | ||
| employeeId?: string | ||
| type UseEmployeeDetailsFormSharedProps = { | ||
| withSelfOnboardingField?: boolean | ||
| optionalFieldsToRequire?: EmployeeDetailsOptionalFieldsToRequire | ||
| defaultValues?: Partial<EmployeeDetailsFormData> | ||
| validationMode?: UseFormProps['mode'] | ||
| shouldFocusError?: boolean | ||
| } | ||
|
|
||
| export type UseEmployeeDetailsFormProps = | ||
| | (UseEmployeeDetailsFormSharedProps & { companyId: string; employeeId?: never }) | ||
|
Comment on lines
54
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Major | Significant undocumented change: UseEmployeeDetailsFormProps was refactored from a single interface to a discriminated union type. This changes the hook's public API contract (callers can no longer pass both companyId and employeeId together). This breaking change to the hook interface is not mentioned in the PR description. |
||
| | (UseEmployeeDetailsFormSharedProps & { employeeId: string; companyId?: never }) | ||
|
|
||
|
Comment on lines
+57
to
+60
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i worry a little bit about this for the component usage 🤔 ex. if you are in create mode, you provide the companyId, then you need to remove the company id once you have the employee id? This is the case you get in employee profile in the partial update case. You set the company id on the hook and then optionally supply the employee id after the update is successful to make sure it gets placed into update mode. In that case, this would introduce the overhead of also needing to omit the company id when the employee id got supplied Profile usage: I think if i was articulating the proper scenarios here Create mode
Update mode
So basically i think that company id should be required if no employee id is present. But if an employee id is present, company id can also be there.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| export interface EmployeeDetailsFields { | ||
| FirstName: typeof FirstNameField | ||
| MiddleInitial: typeof MiddleInitialField | ||
|
|
@@ -182,6 +184,9 @@ export function useEmployeeDetailsForm({ | |
| let updatedEmployee: Employee | ||
|
|
||
| if (isCreateMode) { | ||
| if (!companyId) { | ||
| throw new SDKInternalError('companyId is required to create an employee') | ||
| } | ||
| const result = await createEmployeeMutation.mutateAsync({ | ||
| request: { | ||
| companyId, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.