-
Notifications
You must be signed in to change notification settings - Fork 4
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
431 expand and use button component #490
base: main
Are you sure you want to change the base?
Conversation
…com/IT-Academy-BCN/ita-profiles into 431-expand-and-use-button-component
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.
👍 Good job
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.
La PR resulta casi imposible de revisar debido a la cantidad de cambios no relacionados. Hay además muchos estilos y un defaultButton que se setea a false sin setear otro después. Necesita clarificación
@@ -22,31 +22,8 @@ describe('EditStudentProfile modal opened from MyProfileStudentDetailCard', () = | |||
expect(form).not.toBeInTheDocument() | |||
}) | |||
|
|||
test('should render the modal when user clicks the pencil button', () => { |
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.
Este test no debería estar relacionado con el button
}) | ||
}) | ||
|
||
describe('Modal screenshot', () => { |
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.
No esta relacionado con el button
defaultButton && defaultButtonStyles, | ||
outline && outlineButtonStyles, | ||
navbar && navbarButtonStyles, | ||
close && closeButtonStyles, |
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.
Por favor, enlaza en la UI donde esta este estilo.
secondary && secondaryButtonStyles, | ||
defaultButton && defaultButtonStyles, | ||
outline && outlineButtonStyles, | ||
navbar && navbarButtonStyles, |
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.
Por favor, enlaza en la UI donde esta este estilo.
) | ||
// eslint-disable-next-line no-console | ||
console.log('response de register =>', response.data) | ||
await axios.post('//localhost:8000/users/register', data) |
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.
Utiliza fetch y saca el fetcher a una función externa
<p className="text-error">{`${errors.username?.message}`}</p> | ||
)} | ||
</div> | ||
<Modal isOpen={isOpen} onClose={onClose}> |
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.
Estos cambios no deberian estar incluidos en esta PR
), | ||
)} | ||
return ( | ||
<div data-testid="AdditionalTrainingCard"> |
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.
Estos cambios no deberían de estar por el Button
@@ -8,18 +9,17 @@ const StudentsFiltersModal: React.FC<TStudentsFiltersModal> = ({ | |||
handleOpenModal, | |||
}) => ( | |||
<dialog id="filtersModal" className="modal modal-open modal-bottom flex"> | |||
<div className="modal-box bg-white shadow-sm w-auto flex-1 flex flex-col gap-4 mx-4 p-8 pt-12 pb-5"> | |||
<div className="modal-box bg-white w-auto flex-1 flex flex-col gap-4 mx-4 p-8 pt-12 pb-5"> |
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.
Más cambios no relacionados a Button
<div className="flex justify-between"> | ||
<h3 className="text-lg font-bold">Proyectos</h3> | ||
<div className="h-3 self-end"> | ||
<Button defaultButton={false} onClick={scrollLeft}> |
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.
Un poco extraño el estilo <Button defaultButton={false} onClick={scrollLeft}>
dejarlo a false
No description provided.