search

ZJTAN97 / job_poc-form-component

Proof Of Concept for Custom Form Component with React Hook Form and Zod
https://zjtan97.github.io/job_poc-form-component/
0 stars 0 forks source link

"Code review" #5

Open wenchonglee opened 1 year ago

wenchonglee commented 1 year ago

Form

Form Wrapper

We might want to consider consolidating useLocalStorage and preventLeaving into a config object. But until we have useLocalStorage complete, we can leave this as-is.

No other comments

TextInput, TextArea, Dropdown, MultiSelect

I don't think there's a case for overriding onChange, I'd just omit onChange and value altogether. Then there's no need to useCallback anymore. https://github.com/ZJTAN97/job_poc-form-component/blob/ccb05c50d0e2a07312c37309fa504bdcfd2cee6c/src/components/Form/TextInput/TextInput.tsx#L19-L26

With the change above, there isn't really a need to do this but for clarity sake, spread the {...mantineTextInputProps} before value and onChange. We might even want to consider omitting error from the component prop type. https://github.com/ZJTAN97/job_poc-form-component/blob/ccb05c50d0e2a07312c37309fa504bdcfd2cee6c/src/components/Form/TextInput/TextInput.tsx#L29-L34

Chip

A lint rule we have is to prefix generics with T. https://typescript-eslint.io/rules/naming-convention/#enforce-that-type-parameters-generics-are-prefixed-with-t
A rule of thumb I'm advocating for is:

https://github.com/ZJTAN97/job_poc-form-component/blob/ccb05c50d0e2a07312c37309fa504bdcfd2cee6c/src/components/Form/ChipSelection/ChipSelection.tsx#L11

I think this component will need some design work if we're introducing it, so I won't give more comments than this for now.

wenchonglee commented 1 year ago

Models

https://github.com/ZJTAN97/job_poc-form-component/blob/ccb05c50d0e2a07312c37309fa504bdcfd2cee6c/src/model/career/Career.ts#L66-L77

wenchonglee commented 1 year ago

*I'll pretty much be ignoring the UI (e.g. CareerHistoryContent and any styles in the form) and only looking at the core logic. We'll go through this more thoroughly internally

Career Form (Still reviewing)

https://github.com/ZJTAN97/job_poc-form-component/blob/ccb05c50d0e2a07312c37309fa504bdcfd2cee6c/src/pages/EmployeeInfo/components/CareerHistoryForm/CareerHistoryForm.tsx#L54-L72

https://github.com/ZJTAN97/job_poc-form-component/blob/ccb05c50d0e2a07312c37309fa504bdcfd2cee6c/src/pages/EmployeeInfo/components/CareerHistoryForm/CareerHistoryForm.tsx#L102-L106

wenchonglee commented 1 year ago

StringArrayInput

// from
export const useReferenceStateContext = () => React.useContext(ReferenceStateContext);

// to
export const useReferenceStateContext = () => {
  const context = React.useContext(ReferenceStateContext);
  if(context === undefined) throw "Context must be used within provider";

  return context;
}

If you want to be super type-safe, then you can opt to introduce your own provider

// from (having consumers invoke the context directly)
// where the value is typed to accept undefined
<ReferenceStateContext.Provider value={referenceStateMethods}>

// to exposing a helper (this is pseudo code)
// where you can enforce the type to not accept undefined
export const ReferenceStateProvider = ({ children, methods }: {children:ReactNode, methods: ReferencesStateMethods}) => {
  return <ReferenceStateContext.Provider value={referenceStateMethods}>
    {children}
  </ReferenceStateContext.Provider>
}
let index = 0;
const reindexedRef = references.map(ref => {
  if(ref.field === name) {
    const newRef = {...ref, content: String(index)}
    index +=1;
    return newRef
  }
  return ref;
})

ObjectArrayInput

wenchonglee commented 1 year ago

References

// to const { updateReference } = useUpdateReferences(); const handleMassApply = handleSubmit((formData) => { updateReference({ source:formData, field, arrayId }); // ...redacted });


- Personally, I stop myself when there are 3 or more states I find that I need to wrangle, especially if I'm setting multiple states at the same time. I prefer using a reducer so that the intent becomes clear
  - Let me know if you're unfamiliar with reducers and I can explain it to you. This isn't the most important thing here but I think it helps maintainability **if** we all have a good understanding of reducers
```ts
// from
const [openPanel, setOpenPanel] = React.useState(false);
const [currentField, setCurrentField] = React.useState<Path<CareerType> | Path<AppointmentType> | Path<CertificationType>
>();
const [currentArrayId, setCurrentArrayId] = React.useState<number>();
const [lastSource, setLastSource] = React.useState<SourceType>();
const [isMassApply, setIsMassApply] = React.useState(false);

  // ...snippet of using it in panel.tsx
    setOpenPanel(false);
    setIsMassApply(false);
    handleMassApplyingFields.setState([]);

// to (this is pseudo code I lifted elsewhere)
function reducer(state, action) {
  switch (action.type) {
    case 'RESET':
      return INITIAL_STATE;
    case 'OPEN_PANEL':
      return {openPanel: true, currentField: action.currentField, currentArrId: action.arrId};
    default:
      throw new Error();
  }
}

const [state, dispatch] = useReducer(reducer, INITIAL_STATE);
  // ...snippet of using it in panel.tsx
    dispatch({ type:"RESET"});

  // ...snippet of using it in trigger.tsx
    const handlePanelOpen = () => dispatch({ type:"OPEN_PANEL", currentField, arrId });

Preventing mutations

edit: it seems the author of rhf has commented it is okay to use reset for this purpose. If you find any issues with reset, then we might have to consider rewriting this bit of logic to setValue in the correct places