-
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
[EN-6289] feat(cv): add formation fields experience #139
[EN-6289] feat(cv): add formation fields experience #139
Conversation
1cfb2ac
to
d02f678
Compare
d02f678
to
96090e4
Compare
|
||
interface ExperiencesProfileCardProps { | ||
experiences: CVExperience[]; | ||
onChange: (arg1: Partial<CV>) => void; |
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.
En fait ici je mettrais plutôt : onChange: (updatedExperiences: { experiences: CVExperience[] }) => void;
. Comme ça on a un type plus précis
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.
ok
|
||
interface FormationProfileCardProps { | ||
formations: CVFormation[]; | ||
onChange: (arg1: Partial<CV>) => void; |
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.
Pareil, ici je mettrais plutôt : onChange: (updatedFormations: { formations: CVFormations[] }) => void;
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.
ok
const [remainingItems, setRemainingItems] = useState<number>(); | ||
|
||
useEffect(() => { | ||
setRemainingItems(5 - experiences.length); |
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.
Je mettrais le 5
dans une constante 😉
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.
ok
const [remainingItems, setRemainingItems] = useState<number>(); | ||
|
||
useEffect(() => { | ||
setRemainingItems(3 - formations.length); |
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.
Je mettrais le 3
dans une constante 😉
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.
ok
value: CVExperience | CVFormation; | ||
sortIndex: number; | ||
items: CVExperience[] | CVFormation[]; | ||
onChange: (arg1: Partial<CV>) => void; |
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.
Je mettrais plutôt : onChange: (updatedCV: { experiences: CVExperience[] } | { formations: CVFormation[] }) => void;
. Comme ça le typage est plus précis
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.
J'ai dû faire un truc un peu plus compliqué que ça ;)
@@ -345,19 +354,96 @@ export const PageCVContent = ({ | |||
> | |||
<H2 title="Expériences" color={CV_COLORS.titleGray} /> | |||
</span> | |||
{/* } */} | |||
<ul> | |||
{cv.experiences.map((experience) => { |
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.
Il faudrait vérifier si les expériences sont bien définis ;)
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.
ok
@@ -58,6 +62,10 @@ export const PageCVContent = ({ | |||
cv, | |||
actionDisabled = false, | |||
}: PageCVContentProps) => { | |||
|
|||
|
|||
console.log(cv); |
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.
À retirer ;)
Pense bien à lancer les commandes eslint
avant de pousser pour être sûr que tous ces trucs soit corrigé ;)
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.
yes bien vu
@@ -234,9 +237,12 @@ export const StyledCVPageContentDetailsContainer = styled.div` | |||
flex-direction: row; | |||
margin: 26px 15px 0; | |||
box-sizing: border-box; | |||
flex-wrap: wrap; | |||
flex: 1 1 0%; |
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.
Qu'est-ce que ça fait ça exactement ?
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.
L'espace dans le container flex, que les éléments prennent toute la place qui leur attribuée
{ | ||
id: 'title', | ||
name: 'title', | ||
component: 'text-input', | ||
type: 'text', | ||
title: 'Intitulé de la formation', | ||
}, | ||
{ | ||
id: 'description', | ||
name: 'description', | ||
component: 'textarea', | ||
title: 'Description', | ||
maxLength: 2000, | ||
}, | ||
{ | ||
id: 'location', | ||
name: 'location', | ||
component: 'text-input', | ||
type: 'text', | ||
title: 'Lieu de formation', | ||
}, | ||
{ | ||
id: 'institution', | ||
name: 'institution', | ||
component: 'text-input', | ||
type: 'text', | ||
title: 'Etablissement / Institution', | ||
}, | ||
{ | ||
id: 'dateStart', | ||
name: 'dateStart', | ||
component: 'datepicker', | ||
title: 'Date de début', | ||
}, | ||
{ | ||
id: 'dateEnd', | ||
name: 'dateEnd', | ||
component: 'datepicker', | ||
title: 'Date de fin', | ||
}, | ||
{ | ||
id: 'skills', | ||
name: 'skills', | ||
title: 'Compétences acquises', | ||
component: 'select-creatable', | ||
isMulti: true, | ||
}, | ||
], |
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.
Il manque la propriété isRequired
sur la plupart des champs non ? Il faudrait pas les rendre obligatoire sinon ça va poser soucis non ?
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.
Non il n'y a que le title qui est required, je l'ajoute t'as raison
{ | ||
id: 'title', | ||
name: 'title', | ||
component: 'text-input', | ||
title: 'Intitulé de la formation', | ||
}, | ||
{ | ||
id: 'description', | ||
name: 'description', | ||
component: 'textarea', | ||
title: 'Description', | ||
maxLength: 2000, | ||
}, | ||
{ | ||
id: 'location', | ||
name: 'location', | ||
component: 'text-input', | ||
title: 'Lieu de formation', | ||
}, | ||
{ | ||
id: 'company', | ||
name: 'company', | ||
component: 'text-input', | ||
title: 'Entreprise', | ||
}, | ||
{ | ||
id: 'dateStart', | ||
name: 'dateStart', | ||
component: 'datepicker', | ||
title: 'Date de début', | ||
}, | ||
{ | ||
id: 'dateEnd', | ||
name: 'dateEnd', | ||
component: 'datepicker', | ||
title: 'Date de fin', | ||
}, |
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.
Il manque la propriété isRequired
sur la plupart des champs non ? Il faudrait pas les rendre obligatoire sinon ça va poser soucis non ?
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.
idem
No description provided.