-
Notifications
You must be signed in to change notification settings - Fork 1
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
[FE][CPF-65] Add employee - personal details #77
Conversation
@@ -0,0 +1,71 @@ | |||
'use client'; |
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.
when we set the convention of file and component structure/architecture I will get rid of that line of shame 😄
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.
As mentioned on the channel. Let's go for the interface files, hooks (logic), index and component that takes data from the hook
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.
I left that there because the whole page is a form and needs JS interactivity
cc6fcd0
to
1b26b45
Compare
ed22ef0
to
e6fdf0d
Compare
<div className="w-full"> | ||
<WorkflowTopbar /> | ||
<main className="p-8"> | ||
<div className="grid grid-cols-[minmax(200px,1fr),minmax(400px,1100px),1fr]"> |
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.
Assuming it's not a one-time use, perhaps it would be worth adding this grid cold to the theme?
import { useEffect, useState } from 'react'; | ||
import { useForm } from 'react-hook-form'; | ||
|
||
enum PersonalDetailsFormNames { |
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 have established to use consts instead of enums
@@ -0,0 +1,8 @@ | |||
import { FieldValues, RegisterOptions } from 'react-hook-form'; | |||
|
|||
export interface InputProps { |
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.
Should we add some props for the icon at the beginning/end of the field?
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.
I'll add input styles from Figma styleguide in the next PR. I didn't want to make this PR too big with all the extras.
[PersonalDetailsFormNames.email]: string; | ||
} | ||
|
||
export default function PersonalDetails() { |
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.
According to our agreements about components structure, this should be moved to src/components/pages/PersonalDetails/
. Then you can extract the logic to PersonalDetails.hooks.ts
.
import { Fragment } from 'react'; | ||
import { Typography } from '@app/components/common/Typography'; | ||
|
||
const Completed = () => { |
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.
What do you think about moving it to the utils file?
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.
you mean utils but in the context of sideStepper
, so inside src/components/modules/SideStepper/utils
?
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.
Yeah, exactly!
frontend/src/store/peopleStore.ts
Outdated
@@ -0,0 +1,27 @@ | |||
import { AddNewPersonRouteKeys } from '@app/components/modules/EmployeeSideStepper'; |
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.
I am fine with this approach for now, though I believe as this file grows, we should split it into interfaces, utils, store and actions (potentially)
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.
totally agree, ive just changed that
#65