-
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
[Feat/#238] HostApplyPage 폼 react-hook-form 도입 #240
base: develop
Are you sure you want to change the base?
Conversation
@@ -71,7 +71,7 @@ interface Category { | |||
} | |||
|
|||
interface CategorySelectBoxProps { | |||
selectedCategories: Category; | |||
selectedCategories: Category | undefined; |
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.
undefined를 옵셔널로 주신 이유가 있으실까요?
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.
이부분에 undefined을 주지 않으면 CategorySelectBox 컴포넌트를 호출하는 부분에서 selectedCategories을 prop으로 넘겨줄 때 category1만 string값이 부여되어 있고, category2,3은 string | undefined 타입이라 타입에러가 떠서 이런식으로 수정했습니다!
react-hook-form 도입하시느라 고생많으셨습니다. 리액트 훅 폼이 반드시 비제어 컴포넌트를 필요로하는지는 몰랐네요. 위에서 언급했던 이 ImageSelect, DateSelect, TimeSelect 컴포넌트가 굳이 제어 컴포넌트일 필요가 있을까요? 다만 react-hook-form에 대해서 잘 모르는 입장에서 제시한 의견일 뿐! |
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.
처음 도입해보는거다 보니 고려사항도 많고, 생각할것도 많았을 텐데 잘 적용해주시고, PR에 설명도 잘 남겨 주셨네요.
먼저 PR에서 필요한 논의들을 어느정도 거친 후, 회의 등을 한번 진행하며 좀 정할 필요가 있다고 생각이 들었습니다.
DateSelect, TimeSelect, ImageSelect와 같은 내용도 계속해서 고민해보고 의견있으면 리뷰 남겨놓도록 하겠습니다.
고생하셨습니다.
<form onSubmit={handleSubmit(onSubmit)}> | ||
<main css={mainStyle}> | ||
<section css={sectionStyle}> | ||
<QuestionText numberLabel="Q4">픽플에서 사용할 닉네임을 작성해주세요.</QuestionText> | ||
<Controller | ||
name="nickname" | ||
control={control} | ||
rules={{ required: '닉네임을 입력해 주세요.' }} | ||
render={({ 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.
지금 보니 form 안에 main과 footer가 있는게 좀 이상한 것 같습니다.
main안에 form을 두고 그안에 input들과 button을 두는건 어떨까요? footer는 없애구요
더 좋은 구조 있으면 의견 부탁드립니다
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.
header를 제외한 모든 부분이 일단 form에 해당되기도 하고, 기존에는 main, footer만 있었다면 react-hook-form 라이브러리를 사용하면서 반드시 form 태그를 사용해야 해서 가장 간단하고 직관적인 방법이 main, footer를 모두 form으로 감싸는거라 생각하여 이렇게 구현하였습니다. 그리고 현재 main섹션과 footer 사이에 간격이 좀 있는 편이라 구분하기 위해 main과 footer로 나누긴 했는데
form안에 main, footer가 있는게 좀 이상하다면 input, 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.
음 사실 잘 모르겠습니다,,, ㅋㅋ
근데 가장 어색하다고 생각되는 부분은 푸터로 감싸져 있는 버튼인 것 같습니다.
일반적으로 푸터란 페이지 하단에서 페이지에 대한 정보, 연락처, 저작권 등의 정보를 나타내는 영역이라고 생각합니다.
그래서 단순히 하단에 있다고 footer라고 따로 나누기보다는, button을 main 태그 안으로 가져오는 것은 어떨까하는 생각입니다.
근데 정답이 있는건 아니라고 생각해서,,, 일단 한 번 말해봤습니다
<Controller | ||
name="link" | ||
control={control} | ||
rules={{ | ||
required: '올바른 URL 형식을 입력해주세요.', | ||
pattern: { | ||
value: /^(https?:\/\/)?([\w-]+(\.[\w-]+)+\/?)([^\s]*)?$/i, | ||
message: '올바른 URL 형식을 입력해주세요.', | ||
}, | ||
}} | ||
render={({ field }) => ( | ||
<Input | ||
{...field} | ||
placeholder="URL을 첨부해주세요." | ||
isValid={!errors.link} | ||
errorMessage={errors.link?.message} | ||
isCountValue={false} | ||
/> | ||
)} | ||
/> |
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.
프로젝트에서 사용되고 있는 Input들이 모두 form에서 사용된다면 Controller를 Input안으로 옮겨서 Input컴포넌트 자체를 react-hook-form 을 사용하는 Input으로 만드는 것은 어떤가요?
그냥 Input도 필요하다면 Controller를 포함하는 Input을 FormInput이라고 한다던지 해서, Controller로 감싸는 부분을 공통 컴포넌트 안으로 넣어버리는게 좋지 않을까..? 라는 생각인데 어떠신가요?
저도 react-hook-form을 안써봐서 여러 의견 부탁드립니다.
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.
일단 그런 방식도 생각을 해봤는데 안해봐서 되는지는 확인을 해봐야 할 것 같습니다.
그리고 지금처럼 Controller 내부에 Input을 render할 때나 Controller를 Input 안에 넣을 때나 prop의 개수는 거의 비슷할 것 같긴 합니다.
추후에 리뷰기능이나 공지사항 등 모든 Input이 사용되는 곳을 form으로 본다면 괜찮은 방식인것 같습니다. 하지만 그게 아니라면 공통 컴포넌트인 Input은 지금처럼 유지하는게 좋을 것 같습니다.
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.
제가 찾아봤을 때는 (레퍼는 기억이 안나서,, 찾으면 댓글로 추가하겠습니다 ㅠ) 그렇게 컴포넌트 안에 Controller를 함께 포함하는 것이 가능할 뿐 아니라, 일반적으로 이렇게 많이 사용하는 것 같았습니다.
제 생각에도 Input 컴포넌트 자체를 수정하는 것은 좀 아닌것 같고, Input컴포넌트를 Controller로 감싸놓은 FormInput이라는 새로운 컴포넌트를 만드는게 어떨까라고 생각합니다.
console.log(hostApplyState); | ||
|
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.
console은 삭제 부탁드립니다!
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.
넵!
📌 관련 이슈번호
✅ Key Changes
render
속성으로 해당 컴포넌트를 렌더시키는데 이때 렌더되는 컴포넌트는 반드시 ref를 필요로 하는 비제어 컴포넌트여야 합니다.usePostHostApply
훅 내부에서 useRef를 이용해 focus 해주었지만 react-hook-form의 useForm에서 제공하는 setFocus 속성을 통해 쉽게 자동포커스를 적용시킬 수 있습니다.📢 To Reviewers
📸 스크린샷
2024-08-06.2.03.34.mov