-
Notifications
You must be signed in to change notification settings - Fork 2
feat: multi-step-form Funnel 시스템 구현 #3
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
base: 서준
Are you sure you want to change the base?
Conversation
KIMSEUNGGYU
left a comment
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.
수고하셨습니다.
몇가지 리뷰 남겼습니다.
추가로 react-hook-form 을 각 컴포넌트 마다 작성하셨는데 저 같은 경우 복잡한 form 인 경우에만 react-hook-form 을 사용하는 편인데 각각의 스텝(form 엘리먼트) 요소마다 form 으로 제어하셨는데 이유가 있으실까요??
제가 만약 react-hook-form 을 사용했더라면 funnel 있는 컴포넌트에 form provider 으로 감싸고, 각각의 컴포너넌트에서 useForm 으로 각각의 폼 요소를 제어할 수 있도록 할거 같습니다.
해당 방식을 선택한 이유가 궁금합니다. 의견 나눠 보면 좋을거 같아요!! ㅎㅎㅎ
| }; | ||
|
|
||
| const currentStep = getCurrentStep(); | ||
| const stepIndex = steps.indexOf(currentStep); |
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.
currentStepIndex 로 하면 좀 더 명확한 네이밍이 될거 같습니다 ㅎㅎㅎ
| variant={isSubmitting || !isValid ? 'disabled' : 'primary'} | ||
| disabled={isSubmitting || !isValid} |
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.
variant 를 기본적으로 primary 인데 제출중 또는 유효하지 않은 경우 variant 의 스타일을 변경하셨는데 disabled 일때 스타일을 지정하면 해당 중복 검증 로직을 한쪽에서만 관리할 수 있을거 같아요
| <input | ||
| type='radio' | ||
| value='여성' | ||
| className='sr-only' |
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.
오 sr-only 라는걸 처음 보는데 "화면에는 안보이지만, 스크린 리더를 위한 사용자" 를 위한 스타일이군요! 👍👍
| React.useEffect(() => { | ||
| setValue('genre', selectedGenres, { shouldValidate: true }); | ||
| }, [selectedGenres, setValue]); |
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.
저 같은 경우 useEffect 에 함수명을 작성하는걸 선호하는데 함수명을 작성하면 따로 주석 처리 없이 가독성이 좋아진다고 생각해요 (+ 디버깅시 함수명이 나옴)
React.useEffect(
function validateChangeSelectedGenresEffect() {
setValue('genre', selectedGenres, { shouldValidate: true });
},
[selectedGenres, setValue]);| className={`px-3 py-2 rounded-full text-sm transition-colors ${ | ||
| selectedGenres.includes(genre) | ||
| ? 'bg-blue-500 text-white' | ||
| : 'bg-gray-100 text-gray-800 hover:bg-gray-200' | ||
| }`} |
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.
조건에 따른 스타일이 className 에 있다보니깐 가독성을 헤치는 거 같아서 아쉬움이 있습니다.
- clsx, class-variance-authority, tailwind-merge 라이브러리를 활용해서 className 을 좀 더 잘 정의할 수 있을거 같아요
-> shadcn 에서 자주 사용하는 패턴입니다.
|
|
||
| return ( | ||
| <button | ||
| className={`${getButtonClass()}${className}`} |
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.
- shadn 에서 className 관리하는 방식 (cn()) 을 참고하시면 좀 더 깔끔하게 className 을 관리할 수 있을거 같아요!!
|
|
||
| export default function ErrorMessage({ | ||
| message, | ||
| className = '3', |
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.
className = '3', 에서 3의 의미가 무엇인가요???
| message, | ||
| className = '3', | ||
| }: ErrorMessageProps) { | ||
| if (!message) return null; |
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.
!message 은 빈문자열을 방지하기 위한 인가요?
| fallbackPath?: string; | ||
| } | ||
|
|
||
| export default function OnboardingResultGuard({ |
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.
저는 온보딩 퍼널 페이지에서 가드 코드를 해당 컴포넌트 내부에서 작성했는데 이렇게 따로 guard 컴포넌트로 관리하면 괜찮을거 같습니다! ㅎㅎ (사실 추출하기 귀찮아서 안했지만 ㅋㅋㅋ)
| const getButtonClass = () => { | ||
| if (isDisabled) return 'btn btn-disabled'; | ||
| if (variant === 'secondary') return 'btn btn-secondary'; | ||
| return 'btn btn-primary'; | ||
| }; |
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.
저 같은 경우 이런 경우는 컴포넌트 내부에서 객체 상수로 추출해서 사용할거 같아요
함수가 리렌더링 될 때마다 해당 함수가 계속 생성되고 호출되어서 불필요한 리소스가 낭비가 될 수 있는 포인트라고 생각합니다.
|
|
||
| <Funnel step={currentStep}> | ||
| <Funnel.Step name='nickname'> | ||
| <NicknameStep data={onboardingData} onNext={handleStepComplete} /> |
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.
onboardingData를 다 넣는거보다 각 스텝에 필요한 데이터만 넣는다면 불 필요한 리렌더링이 줄어들 것 같아요! props도 관리하기가 더 편할 것 같습니다ㅎㅎ
| currentStep, | ||
| totalSteps, | ||
| className, |
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.
현재 ProgressBar는 currentStep/totalSteps라는 네이밍 구조 때문에 funnel 전용으로 보여서 features/funnel/components 등으로 옮기는 게 응집도 측면에서 더 자연스러울 것 같아요.
재사용성을 염두하고 있다면 좀 더 범용적으로 value, max 같은 props 네이밍으로 고려해봐도 될 것 같습니다!
|
|
||
| <Funnel step={currentStep}> | ||
| <Funnel.Step name='nickname'> | ||
| <NicknameStep data={onboardingData} onNext={handleStepComplete} /> | ||
| </Funnel.Step> | ||
|
|
||
| <Funnel.Step name='gender'> | ||
| <GenderStep | ||
| data={onboardingData} | ||
| onNext={handleStepComplete} | ||
| onPrev={goToPrevStep} | ||
| /> | ||
| </Funnel.Step> | ||
|
|
||
| <Funnel.Step name='genres'> | ||
| <GenresStep | ||
| data={onboardingData} | ||
| onNext={handleStepComplete} | ||
| onPrev={goToPrevStep} | ||
| /> | ||
| </Funnel.Step> | ||
|
|
||
| <Funnel.Step name='favorite'> | ||
| <FavoriteStep | ||
| data={onboardingData} | ||
| onPrev={goToPrevStep} | ||
| onComplete={handleFinalSubmit} | ||
| isSubmitting={onboardingMutation.isPending} | ||
| /> | ||
| </Funnel.Step> | ||
| </Funnel> |
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
폴더 구조
제출 전 체크리스트
🔗 배포 링크 : https://multi-step-form-lac-six.vercel.app/
리뷰 요청 & 논의하고 싶은 내용
💬 리뷰를 통해 중점적으로 논의하고 싶은 부분
💭 설계 / 구현 단계에서 가장 많이 고민했던 문제와 해결 과정에서 얻은 것
✅ 리뷰어 체크 포인트
1. Form 상태 관리 & 반복되는 로직 분리
2. 컴포넌트 구조 및 재사용성
3. 상태 기반 유효성 검사 및 확인 버튼 활성화