[2단계 - payments module] 쿠키(김진호) 미션 제출합니다. #55
Conversation
There was a problem hiding this comment.
안녕하세요 쿠키~~
다시 만나서 반갑습니다 ㅎㅎ
이번 리뷰는 특별히 코멘트 남길 내용이 많지 않았네요 👍 👍 👍
스토리북과 RTL
정량적으로 점수를 평가하기는 어려운데요. 😅
전체적으로 꼼꼼하게 스토리북과 RTL을 작성하기 위해 노력하신 모습이 보여 좋았습니다
Storybook은 쿠키가 말씀하신대로 예측 가능한 스토리를 보여주는 역할도 하지만 그 자체를 문서처럼 사용할 수도 있는데요. 이런 부분은 조금 아쉽게 느껴졌습니다.
Storybook은 혼자서 사용할 때보다 누군가와 공유하고 협업하기 위한 목적으로 더 많이 사용되는만큼 이런 부분들도 고려해서 작성해보셔도 좋을거 같네용 ㅎㅎ
여기 Storybook이 이런 목적에 부합하게 잘 만들어진거 같은데 참고해보셔도 좋을거 같네용
RTL은 테스트를 명확하고 꼼꼼하게 작성해주신 부분이 아주 좋았습니다. describe으로 컨텍스트를 분리해 나눠놓으신 부분도 좋았구요 👍 👍 👍 소소하게는 현재 describe으로 나눠 둔 컨텍스트들을 context 같은 이름으로 명명해서 나눈다면 가독성이 조금 더 좋아질거 같네요.
카드 번호 포메팅에 대한 고민
이 부분은 사용자에게 더 자유도를 줘도 괜찮을거 같다는 생각도 들어요. Hook으로는 특정 형식에 대한 validation을 제공하고 이 라이브러리를 사용하는 사람이 하나의 Input으로 핸들링 할지 N개의 Input으로 핸들링 할지 결정하는 식으로요.
지금 방식이 잘못 되었다는건 아니니 변경하실 필요는 없고 제 의견일 뿐이니 참고만 해주시면 좋을거 같네요
미션 진행하느라 고생 많으셨습니다 쿠키~~ 즐거운 주말 보내세요~ 👍 👍 👍
components/src/lib/index.css
Outdated
There was a problem hiding this comment.
storybook이 지금은 단순히 모양을 보여주는 용도로만 사용되고 있는데요. docs 같은 것들을 추가해서 해당 컴포넌트를 어떻게 사용할 수 있는지도 같이 보여준다면 더 좋을거 같네요!
There was a problem hiding this comment.
스토리북 docs를 추가해서 사용자가 더 쉽게 스토리북으로 어떻게 사용할 수 있는지 같이 보여줄 수 있도록 표현해봤습니다.
234af26
There was a problem hiding this comment.
오 docs를 아주 멋지게 적용해주셨군요 ㅎㅎ 아주 좋네요! 👍 👍 👍 💯
| @@ -63,8 +120,48 @@ const useCardNumbers = (initialValue: Record<string, string>, options?: CardNumb | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
| }, []); | |||
|
|
|||
| const cardNumbersToArray = Array.from(Object.values(value)); | |||
|
|
|||
| const cardTypeCondition: Record<CardBrand, boolean> = { | |||
There was a problem hiding this comment.
코드만 보고는 어떤 컨디션을 가졌을 때 어떤 결과가 반환되는지 알기 힘든 부분들이 있는데요.
JSDoc 같은걸 이용해서 설명을 같이 남겨도 좋을거 같네용
There was a problem hiding this comment.
ex)
/**
* @example UnionPay('12xxxxx') // true
* @example UnionPay('34xxxxx') // false
*/
There was a problem hiding this comment.
아하 그렇군요! 설명을 추가하면 자세한 조건을 더 잘 이해할 수 있을 것 같네요
e6045fb
hooks/src/lib/index.ts
Outdated
There was a problem hiding this comment.
사용하는 곳에서 선언하는 타입들이 있고 이렇게 모아놓은 타입들이 있는데 어떤 차이가 있는걸까요?
There was a problem hiding this comment.
index에 모아서 export를 하면 라이브러리를 사용하는 측에서 정의해둔 타입을 사용할 수 있더라구요! return type을 따로 정의하지 않고 사용했을 때, 사용 측에서 다시 정의해줘야 하는 일이 일어나 서, 이렇게 반환타입을 정의해두었습니다. 그래서 사용측에서 반환 타입을 import 함으로써 간단하게 타입 정의를 할 수 있도록 한 의도였습니다!
| } as unknown as React.ChangeEvent<HTMLInputElement>, | ||
| 'second', | ||
| ); | ||
| describe('업데이트 검사', () => { |
There was a problem hiding this comment.
jest에서는 context가 없더라구요.. 그래서 describe title 앞에 context prefix를 넣어줘서 context임을 명시해주는 형식으로 변경해봤습니다!
34f4dab
| <Button role="close" {...closeButton} /> | ||
| <Button role="confirm" {...confirmButton} /> |
There was a problem hiding this comment.
CloseButton과 ConfirmButton을 하나의 버튼으로 추상화하면서 각 버튼마다 스타일과 label이 달라져야 할 것 같아서 외부에서 role을 넣어주면 용도에 맞게 디자인이 셋팅이 되도록 설계했습니다
| if (customButton !== undefined) { | ||
| return customButton; | ||
| } |
There was a problem hiding this comment.
customButton을 사용하게 되면 사실상 이 Button 컴포넌트의 코드에서 사용하는 부분이 없어 Button 컴포넌트를 사용하는게 의미가 없을거 같다는 생각이 드네요 😅
There was a problem hiding this comment.
제 생각은 사용자가 커스텀하게 버튼을 넣어주고 싶을 땐 자유롭게 넣어줄 수 있도록 자유도를 주었으며, 버튼을 넣어주지 않았을 때, default로 보여주는 버튼을 보여주도록 한 것이었습니다.
개발의 편의성을 위해서 기본으로 버튼을 보여주며, 더 높은 자유도를 위해서 커스텀한 버튼을 넣어줄 수 있도록 설계한 것이라 커스텀 버튼을 옵션으로 넣어줬습니다.
다시 읽어보니 Button안에 customButton의 로직을 분리하는 것의 의미인 것 같아서 분리했습니다!
62f603e
|
안녕하세요 유조! 주말 잘 보내셨나요? 이번 리팩토링에서 집중한 점은 스토리북입니다. 좋은 레퍼런스도 알려주셔서 감사합니다! 앞으로 스토리북들을 살펴보며 스토리북을 어떻게 보여주면 좋을지에 대해 고민해보겠습니다👍👍 훅에 대해서도 고민을 해봤는데 사용자가 1개 N개 선택에 따라 다르게 제공해준다면 확실히 사용자 입장에서 좋겠지만;; 뭔가 선택적으로 제공했을 때 내부 구현이 복잡해질 것 같아서 하나만 선택해서 제공하는 것이 좋다고 생각했습니다.!! 이번에도 반영한 내용에는 커밋 번호를 달았고, 추가로 의견은 리코멘트 달았습니다. |
| /** | ||
| * @example Diners('36xx xxxx') true | ||
| * @example Diners('35xx xxxx') false | ||
| * | ||
| * @example AMEX('34xx xxxx') true | ||
| * @example AMEX('37xx xxxx') true | ||
| * | ||
| * @example UnionPay('6221 16xx') true | ||
| * @example UnionPay('6229 26xx') false | ||
| * @example UnionPay('624x xxxx') true | ||
| * @example UnionPay('626x xxxx') true | ||
| * | ||
| * @example VISA('4xxx xxxx') true | ||
| * | ||
| * @example MasterCard('51xx xxxx') true | ||
| * @example MasterCard('56xx xxxx') false | ||
| */ |
| @@ -3,7 +3,7 @@ import useCVC from '../lib/useCVC'; | |||
| import { ValidationResult } from '../lib/types'; | |||
|
|
|||
| describe('useCVC 커스텀 훅 테스트', () => { | |||
| describe('초기값 설정 검사', () => { | |||
| describe('context: 초기값 설정 검사', () => { | |||
There was a problem hiding this comment.
context라는 이름으로 describe의 역할을 하게 하는 방법도 많이 사용하니 참고해보시면 좋을거 같아요 ㅎㅎ
나름 높은 다운로드 수를 기록하고 있는 jest-plugin-context 라는 라이브러리가 있는데 코드를 보면 정말 간단하게 구성된걸 확인할 수 있는데요.
이번 미션을 통해 라이브러리를 배포도 해보셨으니 이런 것도 하나씩 만들어서 배포해봐도 재밌을거 같네요 ㅋㅋ
| {!closeButton?.customButton && <Button role="close" {...closeButton} />} | ||
| {!confirmButton?.customButton && <Button role="confirm" {...confirmButton} />} | ||
| {!closeButton?.hide && closeButton?.customButton} | ||
| {!confirmButton?.hide && confirmButton?.customButton} |
|
추가로 변경 된 사항들을 커밋과 함께 남겨줘서 확인하기가 정말 좋았습니다 감사합니다 쿠키 ㅎㅎ 👍 👍 👍 |
안녕하세요 유조! 이번 주 잘 보내셨나요?
연휴 기간 동안에는 우중충하다가 주중에 날씨가 개서 재밌게 잘 보냈던 것 같아요!
2단계 pr도 잘 부탁 드립니다👍👍
🎡배포주소
🗃️구조
🤔이번 미션을 하면서 고민한 점과 질문
스토리북과 RTL
스토리북 다양한 상태와 동작에 대한 예측 가능한 스토리를 보여주어야하고
RTL은 각 커스텀 훅의 책임과 경계를 명확히 하는 테스트를 작성하는 것이 효과적인 작성방법이라고 배웠습니다. 이번에 작성한 스토리북과 RTL이 위와 같이 작성되었는지를 위주로 리뷰를 해주시면 감사하겠습니다😊 제 스토리북과 RTL이 위를 기준으로 했을 때 10점 만점 중에 몇 점인지 평가를 부탁 드리며 어떤 점을 개선한다면 점수를 올릴 수 있을지 알려주실 수 있는지 여쭤봅니다!
카드 번호 포메팅에 대한 고민
이번 미션 요구사항 중에서
카드 번호 포메팅을 해야 하는 요구사항이 있습니다.이에 대해 크루들과 서로 이야기 해봤을 때 다른 크루들은 사용자 측에서 input이 한 개 있다고 생각하여 하나의 input 안에서 포메팅을 하는 기능을 주로 생각했습니다. 하지만 저는 input이 여러 개있는 구조 그대로 가져가서 특정 카드 브랜드가 인식되었을 때, 사용자 측에서 input field가 동적으로 줄어드는 ui를 생각해서 그렇게 구현을 했습니다. 포메팅에 대한 로직을 useCardNumbers 훅의 getInputMaxLengthByCardBrand 함수를 이용해서 사용자 측으로 포메팅을 맡겨버리는 방식으로 구현했습니다.
Hooks.Module.-.Chrome.2024-05-10.21-56-36.mp4