Skip to content
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

[정봉찬] Week20 #1089

Merged

Conversation

devwqc
Copy link
Collaborator

@devwqc devwqc commented May 14, 2024

요구사항

💧기본

  • 변경된 api를 활용해 주세요.
  • 모달에 필요한 api 요청을 만들어 기능을 완성해 주세요.
  • api 요청에 TanStack React Query를 활용해 주세요.
  • Github에 위클리 미션 PR을 만들어 주세요.
  • React, Next.js를 사용해 진행합니다.

주요 변경사항

  • 공통 컴포넌트 Input을 만들었습니다.

멘토에게

  • 이번에 팀 자체적으로 공통 컴포넌트 만들기 연습을 했습니다. Input을 만들어 봤고 다양한 커스텀이 가능하게 합성 컴포넌트 패턴을 적용했습니다.
  • 지금까지 공통 컴포넌트로 Input을 몇 번 만들어 봤는데 조금씩 부족한 부분이 있었습니다. 이번에 만든 Input은 그런 부분을 많이 해소해 주지 않을까 생각이 듭니다.
  • 전에 중첩 CSS에 대해서 질문 드렸을 때 말씀해 주셨던 @layer를 도입해서 완성했습니다.
  • 제가 만든 Input이 공통 컴포넌트의 역할을 제대로 수행할 수 있을 지 멘토님의 의견 및 조언이 필요합니다!

Copy link

vercel bot commented May 14, 2024

@devwqc is attempting to deploy a commit to the soohwan93's projects Team on Vercel.

A member of the Team first needs to authorize it.

@devwqc devwqc requested a review from clianor May 14, 2024 07:27
@devwqc devwqc self-assigned this May 14, 2024
@devwqc devwqc added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label May 14, 2024
@devwqc devwqc marked this pull request as ready for review May 14, 2024 07:28
@devwqc devwqc changed the base branch from main to part3-정봉찬 May 14, 2024 07:40
Comment on lines +63 to +80
for (const key in formData) {
try {
await formSchema.validateAt(key, data);

setFormData((prev) => ({
...prev,
[key]: { ...prev[key], isError: false, message: '' },
}));
} catch (error) {
if (error instanceof Yup.ValidationError) {
setFormData((prev) => ({
...prev,
[key]: { ...prev[key], isError: true, message: error.message },
}));
return;
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 코드를 아래의 코드를 이용해서 개선해 보심이 어떨까요?!

Suggested change
for (const key in formData) {
try {
await formSchema.validateAt(key, data);
setFormData((prev) => ({
...prev,
[key]: { ...prev[key], isError: false, message: '' },
}));
} catch (error) {
if (error instanceof Yup.ValidationError) {
setFormData((prev) => ({
...prev,
[key]: { ...prev[key], isError: true, message: error.message },
}));
return;
}
}
}
formSchema.validate(data, { abortEarly: false }).catch((error) => {
return Object.fromEntries(
error.inner.map((err: Yup.ValidationError) => [err.path, err.message]),
);
});

Comment on lines +11 to +15
type FormValues = {
email: string;
password: string;
passwordConfirm: string;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저 같은 경우는 FormValues는 항상 schema를 통해서 생성하려고 노력합니다.
그래야 한 부분의 코드만 수정해도 다른 부분을 수정하지 않아도 괜찮더라구요.
코드가 미치는 영향도를 조금 고민해보면서 코딩해보셔도 좋을것 같습니다!

Comment on lines +42 to +45
defaultValues: {
email: '',
password: '',
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaultValues를 아에 넣지 않으시거나 혹은 passwordConfirm도 추가해서 일관성을 유지해주시면 어떨까 싶습니다!

Comment on lines +54 to +70
<Input>
<Input.Label className={cx('label')} htmlFor="email2">
이메일
</Input.Label>
<Input.Box isError={errors.email ? true : false}>
<Input.Field
className={cx('inputField')}
type="email"
id="email2"
placeholder="이메일을 입력해 주세요."
{...register('email')}
/>
</Input.Box>
{errors.email?.message && (
<Input.Message>{errors.email.message}</Input.Message>
)}
</Input>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

멘토링때 말씀드렸던 것처럼 공통 Input 컴포넌트를 사용하되 좀더 간결하게 사용할 수 있도록 변경해보심은 어떨까요?!

Comment on lines +7 to +12
const Input = Object.assign(InputLayout, {
Label: InputLabel,
Box: InputBox,
Field: InputField,
Message: InputMessage,
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네이밍을 좀더 명확하게 변경해보심이 어떨까 싶습니다!
그리고 인풋이라는 컴포넌트에 초점을 맞춰보면 Message는 여기에 오는게 적절하지 않아보입니다.

@clianor
Copy link
Collaborator

clianor commented May 14, 2024

멘토에게 답변

컴포넌트를 잘 작성해주셨습니다만 해당 방식으로 컴포넌트를 개발하고 사용하기 위해서는 명확한 인터페이스가 정의되어야 합니다.
Input 컴포넌트를 확인해보면 Input 컴포넌트 내에 다른 컴포넌트들이 어떤 구조로 작성이 될지 모호한점, 그리고 Input의 공통 컴포넌트라면 해당 컴포넌트에 직접적으로 관련된 컴포넌트와 기능들이 제공되어야합니다.

지금 작성된 Input 컴포넌트는 Input 컴포넌트로 보여지기보다는 Form 컴포넌트의 Field의 일부로 보여집니다.
radix-ui의 checkbox가 비슷한 컴포넌트로서 적절한 예시가 될것 같아 첨부드립니다.

링크

@clianor
Copy link
Collaborator

clianor commented May 14, 2024

충돌만 수정하고 다시 올려주세요!

@clianor clianor merged commit 31fec09 into codeit-bootcamp-frontend:part3-정봉찬 May 15, 2024
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants