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

멀티 스텝 온보딩 폼 구현 #18

Merged
merged 10 commits into from
Aug 7, 2024
Merged

Conversation

froggy1014
Copy link
Collaborator

@froggy1014 froggy1014 commented Aug 6, 2024

PR 타입 ( 하나 이상의 PR 타입을 선택해주세요 )


  • 기능 추가
  • 버그 수정
  • 의존성, 환경 변수, 빌드 관련 코드 업데이트
  • 기타 사소한 수정

개요

저희 온보딩 UI에 따라서 일단 API가 안나온 상황에서 더미 데이터로 스텝 로직을 구현해봤습니다.

스탭 버튼 관련해서, 사실 현재 선택된 최소 갯수를 맞아야만 다음 스텝으로 갈 수 있어야하다보니까,

useFormmodeonChange Validation으로 바꿔서.. 컴포넌트 리렌더링은 어쩔 수 없었습니다..

혹시 의문이 가는점이 있다면.. 서슴없이 말씀해주세ㅇ..

코드 리뷰시 참고 사항

preview에서 /onboarding으로 가시면 보실 수 있으실것 같습니다.

https://fiesta-git-feature-onboarding-page-froggy1014s-projects.vercel.app/onboarding

@froggy1014 froggy1014 added the enhancement New feature or request label Aug 6, 2024
@froggy1014 froggy1014 self-assigned this Aug 6, 2024
Copy link

vercel bot commented Aug 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fiesta ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 7, 2024 5:40pm

@froggy1014 froggy1014 changed the base branch from main to develop August 6, 2024 14:09
Copy link

github-actions bot commented Aug 6, 2024

💅 ## storybook-URL - https://66a939e10fecfdf69816c066-zvuupapuyl.chromatic.com/

src/app/onboarding/_components/OnBoardingButton.tsx Outdated Show resolved Hide resolved
src/app/onboarding/_components/OnBoardingCategories.tsx Outdated Show resolved Hide resolved
Comment on lines 38 to 40
const [currentStep, setCurrentStep] = useState<number>(
ONBOARDING.INITIAL_STEP,
);
Copy link
Member

Choose a reason for hiding this comment

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

P4

INTIAL_STEP이 number로 추론 가능하기 때문에 제네릭 <number>는 생략 가능할 거예요
여기 뿐 아니라 다른 곳에서도 유용해요~

Copy link
Collaborator Author

@froggy1014 froggy1014 Aug 7, 2024

Choose a reason for hiding this comment

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

@seokzin

이게 제가 INITIAL_STEP을 초기값으로 상수로 관리하고 있었는데 타입추론이 자꾸

1로 되가지구,, number 타입으로 넣어줘야지 타입스크립트가 인정을 해주더라구요..

감사합니다 ㅎ ㅎ

image

Copy link
Member

@punchdrunkard punchdrunkard left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~~

src/app/onboarding/_components/OnBoardingTitle.tsx Outdated Show resolved Hide resolved
Comment on lines 20 to 31
const getLabel = (step: number) => {
switch (step) {
case 1:
return `다음 (${fields.categories?.length ?? 0}/2)`;
case 2:
return `다음 (${fields.moods?.length ?? 0}/3)`;
case 4:
return `다음 (${fields.priorities?.length ?? 0}/3)`;
default:
return `다음`;
}
};
Copy link
Member

@saseungmin saseungmin Aug 7, 2024

Choose a reason for hiding this comment

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

저는 개인적으로 switch 문을 좋아하지 않습니다. 이유는 로직에 비해 불필요하게 장황해지는 코드, 하나의 옵션을 추가한다면 불필요한 case return break문같은 보일러 플레이트가 많아서 가독성을 해칠 수가 있습니다.

사실 대부분의 클린코드 관련 책에서도 switch 문에 대한 좋지 않은 얘기도 많습니다.

  • 클린코드 책 내용 중 - https://saseungmin.github.io/summary_of_technical_books/docs/clean/clean-code/chapter-3#-switch-%EB%AC%B8
  • 리팩터링 2판 chapter03 코드에서 나는 악취 중
    • 중복된 switch문이 문제가 되는 이유는 조건절을 하나 추가할 때마다 다른 switch문들도 모두 찾아서 함께 수정해야 하기 때문이다. 이럴 때 다형성은 반복된 switch문이 내뿜는 사악한 기운을 제압하여 코드베이스를 최신 스타일로 바꿔주는 세련된 무기인 셈이다.

저는 switch 문 대신 if문을 사용하거나, 비교하는 값이 동일한 경우에는 object로 코드를 개선합니다.

const getLabel = (step: number) => {
  const label = {
    // 숫자를 object 키로 사용할 때에는 주의 필요. 
    // obj[1]과 obj['1'] 동일 동작, 대괄호 표기법(obj[1]) 사용해야함. 
    // Object.keys()로 키를 가져올 때, 모든 키는 문자열로 반환.
    1: `다음 (${fields.categories?.length ?? 0}/2)`,
    2: `다음 (${fields.moods?.length ?? 0}/3)`,
    4: `다음 (${fields.priorities?.length ?? 0}/3)`,
  };
  
  return label[step] || '다음';
};

Copy link
Member

Choose a reason for hiding this comment

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

그리고 undefined 반환으로 0으로 바꿔주는 로직이 중복적으로 발생하는 것 같아요!

export const checkNumber = (value?: number | null): number => {
  if (typeof value === 'number') {
    return value;
  }

  return 0;
};

이런 유틸함수를 만들어주고

const getLabel = (step: number) => {
  const label = {
    // 숫자를 object 키로 사용할 때에는 주의 필요. 
    // obj[1]과 obj['1'] 동일 동작, 대괄호 표기법(obj[1]) 사용해야함. 
    // Object.keys()로 키를 가져올 때, 모든 키는 문자열로 반환.
    1: `다음 (${checkNumber(fields.categories?.length)}/2)`,
    2: `다음 (${checkNumber(fields.moods?.length)}/3)`,
    4: `다음 (${checkNumber(fields.priorities?.length)}/3)`,
  };
  
  return label[step] || '다음';
}

중복되는 분기처리를 개선해볼 수 있을 것 같아요!

src/app/onboarding/_components/OnBoardingCategories.tsx Outdated Show resolved Hide resolved
src/app/onboarding/_components/OnBoardingCompanies.tsx Outdated Show resolved Hide resolved
src/app/onboarding/view.tsx Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Aug 7, 2024

💅 ## storybook-URL - https://66a939e10fecfdf69816c066-fwbemaolvb.chromatic.com/

@froggy1014
Copy link
Collaborator Author

@seokzin
@punchdrunkard
@saseungmin

리뷰 감사드립니다 ㅠ ㅠ

말씀해주신것들은 반영했구요.

클린코드적으로 다시 한번 생각해보게되는... 일단 머지하겠읍니다 🙇‍♂️

@froggy1014 froggy1014 merged commit 091ce9c into develop Aug 7, 2024
5 of 6 checks passed
@froggy1014 froggy1014 deleted the feature/onboarding-page branch August 7, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants