-
Notifications
You must be signed in to change notification settings - Fork 21
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
[나지원] sprint11 #133
The head ref may contain hidden characters: "Next-\uB098\uC9C0\uC6D0-sprint11"
[나지원] sprint11 #133
Conversation
- Installed react-hook-form for login form handling - Created useAuth custom hook to manage authentication logic
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.
지원님 이번 스프린트 미션 고생하셨습니다~
<div className={`${styles.bannerBottomImg} ${styles.img}`}></div> | ||
</div> | ||
</section> | ||
<footer className={styles.footerContainer}> |
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.
P3:
footer는 따로 분리하지 않아도 되나요?
const { list } = await fetchData(BASE_URL, { | ||
query: { keyword: typeof keyword === "string" ? keyword : "" }, | ||
}); | ||
const { list } = await fetchData<Record<string, ArticleProps[]>>( |
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.
P1:
지금 선언해주신 Record<string, ArticleProps[]>
타입이 의미하는 것은 객체의 키가 string 타입이고, 그 값은 ArticleProps 타입을 가진 배열이라는 뜻입니다.
의도하신 바는 아마 list
키값에 ArticleProps[]
타입을 가진 배열이 응답값으로 내려온다는 것이므로 아래 코드가 더 적절해보입니다.
const { list } = await fetchData<Record<string, ArticleProps[]>>( | |
const { list } = await fetchData<{ list: ArticleProps[] }>( |
body: { content: comment }, | ||
} | ||
); | ||
setComment(""); |
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.
P2:
react-hook-form을 사용하시니 해당 form에서 reset 함수를 사용하시면 더 좋을 것 같습니다~
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.
P3:
fetchData가 method 들을 모두 커버하다보니 로직이 커지는 것 같아요.
fetchDatat 라는 하나의 함수로 유지하고 싶으시면 내부 로직을 분리하시는 것을 추천드려요.
const fetchData = async<T>(...) => {
const { method } = options;
switch(method) {
case 'POST':
postData(...);
case 'PUT':
putData(...);
case 'PATCH':
patchData(...);
case 'DELETE':
deleteData(...);
case 'GET':
default:
getData(...)
}
}
error?: FieldError; | ||
} | ||
|
||
const FormInput = <T extends FieldValues>({ |
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.
P3:
react-hook-form의 register 함수를 실행해서 반환값을 받도록 하면 더 깔끔하지 않았을까싶네요.
<div | ||
tabIndex={0} | ||
role="button" | ||
className={styles.dropdownButton} | ||
onClick={toggleDropdown} | ||
> |
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.
P3:
혹시 button 태그가 아니라 div 태그를 쓰시고 tabIndex, onClick을 추가하신 이유가 있을까요?
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.
P3:
react-hook-form을 설치하셨으니 나중에 이 부분도 해당 라이브러리를 사용하는 방향으로 수정하시면 더 좋을 것 같습니다~
register={register} | ||
required="이메일을 입력해주세요." | ||
pattern={{ | ||
value: /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/, |
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.
P2:
정규 표현식이 로그인, 회원가입에서 공용으로 쓰이니 따로 객체에 저장해두시고 같이 쓰시면 더 좋을 것 같습니다.
에러메시지도 여러 페이지에서 반복적으로 사용된다면 동일합니다~
요구사항
기본
회원가입
로그인
메인
심화
변경 사항
배포 사이트
https://najimarket.vercel.app/