-
Notifications
You must be signed in to change notification settings - Fork 208
[1단계 - 페이먼츠] 재오(권재영) 미션 제출합니다. #442
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
Conversation
UX 피드백
|
상태 관리 관련 질문전반적으로 custom hook, context api 등 여러 상태 관리 방식을 먼저 고민해보신 뒤, 현재 앱 사이즈에 맞는 결정을 내리신 부분이 합리적으로 느껴집니다. 상태 관리에 대한 방식이 꽤 많지만, 처음부터 이유도 없이 도입을 시도하는 것보다는 가장 기본적인 상태 관리 방식으로 접근하는 것도 좋은 출발인 것 같아요. 이번 미션에서는 컴포넌트의 재사용성에 좀 더 초점을 맞추고 더 복잡해질 다음 미션부터 엄밀한 상태 관리에 대한 고민을 이어가보시죠 ! Error 상태 관리 관련 질문우선 '하나의 인풋에 대해 여러 에러를 갖고 있어야 하는 이유가 무엇인가?'에 대한 질문이 선행되어야 할 것 같아요. 인풋에 대한 에러는 주로 화면 내에 메시지를 표현하거나, submit 가능 여부를 판가름할 때 사용할 텐데 하나의 인풋에 대한 에러는 가장 우선으로 마주한 에러만 관리하고 있어도 괜찮지 않을까요? Story 관련 질문주로 UI가 특정 상황에서 어떤 식으로 보이는지에 대해 초점을 맞춰, 이러한 테스트로 이점을 얻을 수 있는 상황에서 사용하는 것 같습니다. 예를 들어
컴포넌트 분리 기준 관련 질문이 부분은 코드 레벨로 확인해볼게요! config 폴더 관련 질문현업 내에서도 데이터를 어떻게 다룰 것인가에 대한 방식이 상이합니다. 그래서 팀 내 컨벤션으로 규정해두는 경우가 많은데, 저 같은 경우 도메인 관련 타입, 상수 또한 모두 비즈니스 로직의 일부로 보기 때문에 '연관된 비즈니스 로직을 가깝게 두어 집중화하는 방식'을 추구해요. 이름은 크게 어색하지는 않지만 특정 |
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.
normalize.css 를 고려해보셨을까요? reset.css를 사용한 이유가 궁금하네요 ~
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.
이번 미션이 아니라 저번 미션에서 두 가지 사용을 고려해본 적 있습니다.
normalize.css는 'CSS Normalize는 가능한한 브라우저의 내장 스타일을 최대한 건들지 않는 선에서 브라우저 간에 상이한 부분만 스타일을 통일시켜주는 것'이었습니다.
즉, normalize.css 브라우저의 내장 스타일을 초기화해주지 않아서 "저희가 의도한 스타일만"을 적용하기 어려울 것이라고 판단이 되어, reset.css을 사용하게 되었습니다!
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.
오호 그렇군요 공감되는 이유입니다. 다만 정말 독창적인 디자인의 앱을 만들지 않는 이상 대부분은 normalize.css를 사용하는 선에서 출발하는 게 초기 스타일 작업량을 줄일 수 있기 때문에 둘 다 써보시고 추후 팀 프로젝트 때 뭘 써야할지 결정의 근거로 삼아보시길 바라요!
src/config/card.ts
Outdated
| @@ -0,0 +1 @@ | |||
| export type CardType = 'visa' | 'master' | 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.
null은 타입에 들어가기보다 실제 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.
null을 타입으로 사용하는 것보다, 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.
앗 그건 아니었고 유니온에 들어갈 타입은 유효한 값만을 두고 '없는 것'에 대한 처리는 사용처에 위임하는 게 좋겠다는 말이었습니다! null, undefined가 핵심 타입으로 들어가게 되면 사용처에서 불필요한 타입 가드를 추가해야할 수도 있기 때문이에요.
type CardType = 'visa' | 'master'
function getCardTypeOrNull (): CardType | null {
if (...) {
return null
}
return cardType
}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.
아 이해했습니다 ㅎㅎ 2단계에 적용해보겠습니다!
src/InputSection.tsx
Outdated
| interface InputSectionProps { | ||
| title: string; | ||
| caption?: string; | ||
| children: ReactNode; |
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.
PropsWithChildren을 사용해볼 수 있을 것 같네요 !
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.
오 PropsWithChildren에 대해서 처음 알게 되었네요. 감사합니다 🙇♂️
해당 내용에 대해서 찾아보았습니다.
type PropsWithChildren<P = unknown> = P & { children?: ReactNode | undefined };children prop이 옵셔널로 받게 되어 있더라구요. children을 안받을 수도 있다는 의미인데, 제 코드에서 children을 받는 컴포넌트를 봤을 때 children을 옵셔널로 받게 되면 조금 어색해질 것 같다고 판단이 되었습니다.
그래서 StrictPropsWithChildren를 만들어 children을 props를 추가하면서 필수로 받게 하는 것을 만들었습니다!
이렇게 진행해도 괜찮을까요?
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.
아하 그런 의도라면 처음의 방식을 그대로 고수해도 괜찮을 것 같습니다 ! 만일 children을 필수로 받는 컴포넌트가 많아서 이를 확실한 규약으로 삼고 싶다면 수정해주신 방향을 따라가면 될 듯 하네요
src/config/inputField.ts
Outdated
| ] as const; | ||
| export type CardNumberInputType = (typeof CARD_NUMBER_INPUT_TYPE)[number]; | ||
|
|
||
| export const EXPIRATION_DATE_INPUT_TYPE = [ |
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, year로 나눠볼 수 있지 않을까요?
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.
cardNumber의 input에 대한 키를 cardNumberPart1, cardNumberPart1, ... 로 선언하게 되어, 통일성을 위해 expirationDatePart1, expirationDatePart2로 나누게 되었습니다.
하지만 카일 피드백도 그렇고 마지막에 해당 키 값을 봤을 때, 직관적이지 않다는 느낌을 받았습니다.
통일성보다는 네이밍에 대한 직관성이 더 중요할까요?
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.
아 두 가지를 같이 가면 가장 좋은 케이스이지만 둘 중에 하나를 택해야 한다면 명확성이라는 의미이군요!
저는 통일성을 최우선으로 보고 있었는데, 카일의 코멘트에 동의하게 되었습니다. 앞으로 카일의 코멘트를 기억하고 적용해보겠습니다!
igy95
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.
안녕하세요 재오! 이번 미션 잘 부탁드립니다~ 전반적으로 이번 미션은 Input의 재사용성에 초점을 맞추어 코드를 살펴봤는데요, Input과 InputField의 역할을 잘 나눠주신 부분이 인상 깊었습니다. UX 피드백 및 질문에 대한 답변, 세부 코멘트 등을 남겨두었으니 확인 후 재요청 부탁드릴게요 ~!
현재는 onChange, onBlur에서 하나의 인풋에 대한 에러 처리를 분리해서 처리하기 때문에 에러의 상태(?)가 조금 꼬인 것 같습니다.
현재는 코드 규모가 그렇게 크지 않아서 domain 별로 폴더를 분리를 하지 않고 진행했습니다. 하지만 storybook 파일도 그렇고 config 파일도 그렇고 특정 domain 하위에서 관리하면 카일이 언급해주신 '연관된 비즈니스 로직을 가깝게 두어 집중화하는 방식'으로 분리가 되어 유지보수가 좋을 것 같다는 생각이 드네요! 좋은 피드백 감사합니다! |
|
안녕하세요 카일! |
igy95
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.
안녕하세요 재오 ~ 리뷰 수정사항과 코멘트 확인 완료했습니다. 우선 코드에 대한 본인의 고민과 리뷰를 남겼을 때 그대로 수용하지 않고 좀 더 응용해보려고 하는 모습이 긍정적으로 느껴지네요 👍 CardType에 대해 추가로 남긴 코멘트만 확인 부탁드리고 다음 단계에서 보시죠 !
안녕하세요 카일!
우테코 7기 FE 재오입니다.
벌써 레벨2의 첫 미션이네요! 이번 미션 리뷰 잘 부탁드립니다! 🙇♂️
🎯 미션 소개
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
2) 이번 리뷰를 통해 논의하고 싶은 부분
변경 전
변경 후
Input,BaseInputField,CardPreview컴포넌트만 Story로 분리해 작성했습니다.✅ 리뷰어 체크 포인트
1. 컴포넌트 설계
2. 상태 관리
3. Storybook 활용
4. UI/UX