-
Notifications
You must be signed in to change notification settings - Fork 69
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
[클린코드 리액트 3기 정진범] 페이먼츠 미션 Step3 #159
base: jungjinbeom
Are you sure you want to change the base?
Conversation
파일 구조 설정 스토리북 셋팅 카드 폼 컴포넌트 생성
요구사항 예외처리 카드폼 컴포넌트 개발
CardBox, CardForm 컴포넌트 구현 CardNumbers 실시간 UI 반영
카드 폼 제어컴포넌트로 수정 유효성 검사 함수 로직 수정
공통 컴포넌트 추가 및 수정 util 스타일 추가
Card 컴포넌트에 사용되는 UI 컴포넌트 수정 카드 리스트 Context 추가 디렉토리 구조 수정
모달에 사용되는 컴포넌트 추가 ModalContext 추가
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.
안녕하세요, 진범님!
코드 변경사항 81개... 바쁘실 텐데 수고 많으셨습니다 👍👍👍
피드백 주신대로 CardListContext와 CardInfoContext를 합쳤는데 현재 작성한대로 관리를 하면 되는지 고민입니다
- 합쳤기 때문에 관심사가 명확히 응집되었는데, 해당 컨텍스트의 역할이 너무 거대해보이긴 합니다.
- 이 시점에서 CardInfoContext를 사용하는 커스텀 훅을 만들어서 제어하는 형태로 리팩토링하면 어떨까요? Context 제공 주체는 CardInfoContext, 그것을 사용하기 좋게 만들어둔 Custom Hook. 이러면 사용자는 훅만 잘 가져다 사용하면 될 거 같거든요!
Context 리렌더링 이슈로 인한 성능저하를 우려해 메모제이션을 사용을 추천해주셨는데 Context 최상단에 선언하여 Props로 넘겨주고 Memo 처리해야하는지 아니면 Context에서 관리하는 함수를 메모제이션 훅으로 관리하면 되는지에 대해 고민이 됩니다.
- 전자도 좋고, 후자도 좋습니다. 어느 정도의 Props나 함수가 메모이제이션 될 지를 염두에 두고 개발하신다면 엄청 효율적으로 리팩토링하실 수 있을 거에요.
- 사실 그만큼 성능 저하가 있는 것도 아닌데! 라는 생각이 드시더라도 학습을 위해서 메모이제이션을 적용하고 어느 부분에서 리렌더링이 방지되고 불필요한지 리액트 개발 툴로 확인해보면 좋을 것 같아요.
이번에는 RC 드릴게요. 내용 읽어보시고 하나씩 수정해보세요!
src/stories/Page.tsx
Outdated
onLogin={() => { | ||
setUser({name: 'Jane Doe'}); | ||
}} | ||
onLogout={() => { | ||
setUser(undefined); | ||
}} | ||
onCreateAccount={() => { | ||
setUser({name: 'Jane Doe'}); |
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.
src/stories는 사용하지 않는 코드니까 제거해도 좋습니다~!
return null; | ||
throw new Error('라우터 컨텍스트 입니다!'); | ||
} | ||
|
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.
👍
그런데 에러 문구가 조금 난해하네요. 해당 컨텍스트는 StepProdiver 하위에서만 사용해주세요 등은 어떨까요?
에러 문구를 보고 어떻게 수정하면 될 지 알려주는 방향이 더 좋아보입니다!
const navigate = (path: Route) => { | ||
setRoute(path); | ||
}; |
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.
route, navigate는 memoization 하여 잦은 리렌더링을 방지하는 하나의 대책으로도 쓸 수 있을 것 같네요!
.eslintrc.js
Outdated
extends: [ | ||
'eslint:recommended', | ||
'plugin:@typescript-eslint/recommended', | ||
// 'plugin:react-hooks/recommended', |
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.
주석을 처리한 이유가 있나요?!
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.
사용하지 않는 룰인데 주석 처리하고 삭제하는걸 깜빡햇습니다
</CardInfoProvider> | ||
</> | ||
)} | ||
<Stepper /> |
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.
훨씬 선언적으로 되었네요 👍
const Input = forwardRef( | ||
( | ||
{ type = 'text', boxType = 'input-basic', className, ...props }: InputProps, | ||
ref: ForwardedRef<HTMLInputElement>, | ||
) => { | ||
return <input type={type} className={classNames(boxType, className)} {...props} ref={ref} />; | ||
}, | ||
) => <input type={type} className={classNames(boxType, className)} {...props} ref={ref} />, |
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.
forwardRef는 제네릭 타입을 지원해서 첫 번째, 두 번째 인자를 잘 정의해주면 ForwardedRef 를 추가적으로 import 하지 않아도 될 것 같아요. 의도한 타입일까요?
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.
주로 사용하던 방법인데 굳이 이렇게 사용하지 않아도 말씀해주신 forwardRef 제네릭 타입을 활용하는게 좋은거 같다고 생각합니다
const Modal = ({ onToggle, isOpen, children }: ModalProps) => | ||
isOpen ? ( | ||
<div className="modal-dimmed" onClick={onToggle}> | ||
<div | ||
className="modal" | ||
onClick={(e) => { | ||
e.stopPropagation(); | ||
}} | ||
> | ||
{children} | ||
</div> | ||
</div> | ||
); | ||
}; | ||
) : 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.
createPortal을 적용해보면 어떨까요?!
src/domain/constant.ts
Outdated
@@ -1,4 +1,10 @@ | |||
export const MIN_MONTH = 1; | |||
export const MIN_MONTH = 0; |
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.
상수를 작성할 때도 고민해보면 좋을 부분이, MONTH는 1~12인데 0이라고?
라는 생각을 제 3자가 가질 수 있다는 겁니다.
어떻게 처리하면 좋을까요?
src/domain/validate.ts
Outdated
export const validNumber = (value: string) => { | ||
return /^(?:[1-9][0-9]{0,2}|)$/.test(value); | ||
}; | ||
export const validNumber = (value: string) => /^(?:[1-9][0-9]{0,2}|)$/.test(value); |
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.
이 함수도 boolean을 반환하므로 아래의 네이밍처럼 일관성을 만들어주면 어떨까요?
export const validNumber = (value: string) => /^(?:[1-9][0-9]{0,2}|)$/.test(value); | |
export const isValidNumber = (value: string) => /^(?:[1-9][0-9]{0,2}|)$/.test(value); |
src/pages/Stepper.tsx
Outdated
const Stepper = () => { | ||
const { route } = useStepContext(); | ||
|
||
if (route === 'LIST') { | ||
return <CardList />; | ||
} | ||
|
||
if (route === 'CARD') { | ||
return ( | ||
<ModalProvider> | ||
<AddCard /> | ||
</ModalProvider> | ||
); | ||
} | ||
|
||
if (route === 'COMPLETE') { | ||
return ( | ||
<ModalProvider> | ||
<CardRegisterComplete /> | ||
</ModalProvider> | ||
); | ||
} | ||
|
||
return <></>; | ||
}; |
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.
이 시점에서 이러한 코드를 깔끔하게 정리하는 좋은 코드 기법을 알려드리자면,
const Stepper = () => { | |
const { route } = useStepContext(); | |
if (route === 'LIST') { | |
return <CardList />; | |
} | |
if (route === 'CARD') { | |
return ( | |
<ModalProvider> | |
<AddCard /> | |
</ModalProvider> | |
); | |
} | |
if (route === 'COMPLETE') { | |
return ( | |
<ModalProvider> | |
<CardRegisterComplete /> | |
</ModalProvider> | |
); | |
} | |
return <></>; | |
}; | |
const StepperContent = { | |
LIST: <CardList />, | |
CARD: ( | |
<ModalProvider> | |
<AddCard /> | |
</ModalProvider> | |
), | |
COMPLETE: ( | |
<ModalProvider> | |
<CardRegisterComplete /> | |
</ModalProvider> | |
), | |
} as const; | |
const Stepper = () => { | |
const { route } = useStepContext(); | |
const Component = StepperContent[route] ?? null; | |
return Component; | |
}; |
이렇게 key-value 쌍으로 만들 수 있답니다!
- 사용하지 않는 커스텀훅 제거 - 스토리북 예제 파일 제거
안녕하세요 인성님!
PR 늦게 올려서 죄송합니다. 우선 최대한 개발해서 PR 올립니다.
나머지 개발되지 않은 부분은 7주차에 추가하려고 합니다.
지금은 피드백 주신 부분을 수정하면서 저번에 말씀해주신대로 두번 이상 핑퐁을 진행하고 싶습니다.
고민거리
필수 요구사항
UX/UI 요구사항
유효성 검증
카드
보안코드 툴팁
가상 키보드