[클린코드 리액트 3기 김기문] 페이먼츠 미션 Step1 #116
Conversation
There was a problem hiding this comment.
안녕하세요 기문님! 페이먼츠 미션을 함께하게 된 박선호라고 합니다 :)
미션 진행하시느라 고생 많으셨어요! 리뷰에 대한 질문이나 더 궁금한점 남겨주시면 빠르게 답변 드리도록 하겠습니다 😆
현재 LMS 페이먼츠 미션 Step 1에 나와 있는 필수 요구사항중 많은 부분이 아직 구현이 안되어있는 것 같아요 🥲 (ex. 카드 번호는 숫자만 입력가능하다, 월은 1이상 12이하 숫자여야 한다 등등) 필수 요구사항들 더 구현해주시고 다시 리뷰요청 주시면 더 꼼꼼하게 리뷰 진행해보도록 하겠습니다!
추가적으로 storybook 상호 작용 테스트가 필수 요구사항인데요, 구현해주신 Input 컴포넌트라도 storybook 작성해보시면 많은 도움이 되실 것 같습니다!
현재 필수 요구사항 중 사용자가 카드등록페이지에서 카드번호 등을 입력하면 form상단에 존재하는 카드 모양의 component에 실시간으로 반영되어야 합니다.
함께 렌더되어야 하는 값과 아닌 값들을 나눠서 생각해보면 좋을 것 같아요. 상태를 어떻게 나눠서 관리할 것인지를 고민해보면 앞으로 구조를 어떻게 잡아갈지에 대한 도움이 될거에요. 함께 변경되어야 하는 값들이 같이 렌더되는 것은 자연스러운 일이니까요! 추가적으로 useCallback과 같은 메모리제이션 관련 함수들에 대해 학습해보면 도움이 되실 것 같습니다
storybook으로 어디까지 테스트를 커버해야한다고 생각하시나요? 아니면 리뷰어님께서는 보통 storybook으로 어디까지 테스트를 커버하시는지 궁금합니다.
주어진 상황에 따라 다를 것 같아요. 저는 스스로 만든 컴포넌트 테스트&개발자가 아닌 분들과를 위한 소통 방법으로 스토리북을 사용하고 있습니다. 스토리북을 통해 prop을 변경해보며 엣지 케이스들에 대한 ui를 확인해보기도 하고 구현이 빠진 부분이 없는지 확인해보는 편입니다. 완벽한 구현 전 디자이너나 QA분들과의 소통 창구로도 잘 사용하고 있습니다!
src/types/Input.tsx
Outdated
| >, | ||
| "onChange" | ||
| > & { | ||
| customType?: "textOnly" | "numberOnly"; |
There was a problem hiding this comment.
input의 type attribute와 중복되는 타입인 것 같은데요, 따로 customType을 선언하신 이유가 있을까요??
There was a problem hiding this comment.
input의 type에서 text의 경우 문자열, 숫자, 특수문자등이 모두 입력가능한 즉, type='text'는 그냥 명시용의 기능밖에 없어 오직 string type만 입력가능한 기능이 필요했습니다. 때문에 textOnly를 추가했고,
numberOnly는
- type='number'는 숫자만 입력가능하게 바뀌지만 input의 우측 끝에 숫자크기를 조절할 수 있는 생기는 버튼들을 원하지 않았습니다.
- 제가 만든 primitive input (Input.tsx)를 보시면 예를들면 카드번호 사이에 대쉬기호가 들어가게 되면 이 또한 모두를 input value값으로 가지게 되어있습니다. 따라서 숫자들 사이에 특수문자들마저 걸러지게 되어서
위 두가지 문제(?)를 해결하기 위해 customType을 만들게 되었습니다
There was a problem hiding this comment.
- 조금 귀찮기는 하지만 css로도 삭제가 가능합니다!
-값이 input에 함께 들어야해서 따로 타입을 정의해주신걸로 이해를 했는데요, 그렇다면 numberOnly라는 타입이 어색한것 아닐까요?? 추가적으로 '*' 값도 함께 들어가는 타입인것 같아 질문드렸습니다. primitive 컴포넌트에서는 최대한 비즈니스 로직에서 먼 값들을 가져가고, 비즈니스 로직을 가진 컴포넌트로 해당 값을 한번 더 감싸는건 어떻게 생각하시나요??
|
@prefer2 첫번째 리뷰에서 코멘트 주신부분 중
월에 1이상 12이하 숫자여야하는 부분은 예를들어 13을 입력하고 계속해서 년을입력하게되면 input값이 초기화되어 최종적으로는 1미만 12초과되는 값은 결과값으로 제출되지 않는 방법으로 해두었었습니다. 혹시 이 방법은 좋지 않은 방법일까요? |
| addons: [ | ||
| "@storybook/addon-links", | ||
| "@storybook/addon-essentials", | ||
| "@storybook/addon-onboarding", |
There was a problem hiding this comment.
해당 addon은 현재 필요한 addon일까요??
추가적으로 storybook 셋팅해주시면서 불필요한 asset과 story가 생성된것 같은데요, 한번 정리해주시면 좋을 것 같습니다!
| export const TextOnly: Story = { | ||
| args: { | ||
| customType: "textOnly", | ||
| }, | ||
| }; | ||
|
|
||
| export const NumberOnly: Story = { | ||
| args: { | ||
| customType: "numberOnly", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
control로 해당 prop을 조정하며 테스트가 가능합니다! (요게 storybook의 매력 같아요 ㅎㅎ) 추가적으로 input을 테스트해볼 수 있도록 value값을 지정해보는것은 어떨까요??
There was a problem hiding this comment.
@MoZZANG 님! 리뷰 반영 감사합니다~!!
남겨주신 코멘트에 추가적으로 궁금한 점들과 storybook 관련 리뷰 추가해두었습니다!
month에는 1~12까지만 입력 가능하다는 점을 여러번 남겨서 혼란을 드린것 같네요ㅠ 해당 부분 수정해주신것 확인했습니다 👍 유효하지 않은 값일 시 input 초기화되는 점에 대해서는 이의 없습니다! (ux를 위해서라면 alert를 띄우거나 빨간색 글씨로 표현해 줄 수 있을 것 같네요ㅎㅎ)
코드와 관련된 점은 아니지만 커밋 단위를 조금 더 작게 나눠주시면 소통하는데 더 도움이 될 것 같아요(한 가지 기능 단위로 커밋 분리, 리뷰 반영건당 커밋 분리 등) 저는 경험상 한 커밋에 세 파일 이상을 담지 않을려고 노력합니다(팀에서는 히스토리 추적을 위해 커밋당 두개 이상의 파일 변경을 하지 않도록 하고 있습니다)
궁금한 점이나 의문이 되는 점들 코멘트 추가적으로 남겨주시면 감사하겠습니다 😆
안녕하세요! 이번에 리뷰어를 맡아주셔서 정말 감사합니다.
부족한 코드 많이많이 리뷰 부탁드립니다😆
현재 PR에 관하여
궁금한점
❓현재 필수 요구사항 중 사용자가 카드등록페이지에서 카드번호 등을 입력하면 form상단에 존재하는 카드 모양의 component에 실시간으로 반영되어야 합니다.
리뷰어님께서 보시기에는 첫번째 영상과 같은 경우는 필연적으로 발생할 수 밖에 없다고 생각하시나요? 아니면 카드번호를 입력하고 있다면 카드번호 input과 Card 컴포넌트만 리렌더링되게 만들 수 있다고 보시는지요?
❓storybook으로 어디까지 테스트를 커버해야한다고 생각하시나요? 아니면 리뷰어님께서는 보통 storybook으로 어디까지 테스트를 커버하시는지 궁금합니다.
##질문사항에 포함된 첨부영상
첫번째 영상
https://github.com/next-step/react-payments/assets/98200337/ea2c1db4-0938-4df6-af30-e8554a93e7f3
두번째 영상
https://github.com/next-step/react-payments/assets/98200337/506ba4d3-bbd9-4db4-bf3b-adc491946ff7