-
Notifications
You must be signed in to change notification settings - Fork 15
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
[넥스트레벨 호준] 페이먼츠 미션 STEP1 #3
base: ganeodolu
Are you sure you want to change the base?
Conversation
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.
호준님 안녕하세요!
pr을 올려주신지 너무나 많은 시간이 지났는데 죄송하게도 이렇게 늦게 리뷰를 해드려서 죄송합니다.
자바스크립트가 탄탄하신 것 때문이신지 코드가 전체적으로 깔끔하다는 생각이 들었습니다.
리액트를 오랜만에 쓰신다고 하셨는데 전혀 못느낄만큼 컴포넌트 분리를 잘해주신 것 같습니다.
( 처음에 page부터 봐서 addCard를 보고있었는데 나중에 보니 다 분리를 해놓으셨더라구요! )
제가 storybook이나 webpack을 잘 몰라서 초기설정하신 것에 관해서는 리뷰를 못드리는 점 양해부탁드립니다 ㅠㅠ 중간중간에 코멘트를 달았으나 제가 부족해서 호준님이 쓰신 코드를 정확히 이해하지 못해 쓴 코멘트가 있을 수 있으니 놓친부분이 있다면 알려주시면 감사하겠습니다! 그리고 잘하고 계시니 시간이 괜찮으시다면 꼭 완성하셨으면 좋겠습니다. 제가 같이 팀원으로써 으쌰으쌰하면서 같이 해야하는데 제가 바쁘다는 이유로 그렇게 해드리지 못해 다시 한번 너무 죄송하다고 말씀드리고 싶습니다. 리뷰가 필요하시다면 언제든 말씀해주세요! 여유가 있을 때 호준님 코드를 보면서 배운것이 많아서 꼭 한번 더 보고싶네요 !
securityCode: '', | ||
}); | ||
|
||
const moveNextFocus = (e) => { |
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.
리액트에서는 dom에 직접적인 접근보다는 ref를 사용하더라구요! 혹시 useRef를 알고계시다면 그것을 한번 적용해보는 것도 좋을 것 같습니다!
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에 커서를 옮겨주기 위해서 직접적인 dom 조작을 하신 것으로 이해가 되었는데 그러면 useRef를 쓰기 조금 까다로울 것 같다는 생각을 했습니다! 구글에 다음과 같이 검색을 해보니 "useRef로 다음 Input focus" 조금 도움이 될만한 글들이 나와서 참고하시면 좋을 것 같습니다! (사실 이게 더 효율적인지는 저도 잘 모르겠어요 ㅠㅠ)
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.
리뷰주신데로 useRef를 이용해서도 시도해보겠습니다. 다양한 방법을 제시해주셔서 감사합니다. 👍🏼
const AddCardContainer = () => { | ||
const history = useNavigate(); | ||
|
||
const [inputs, setInputs] = useState({ |
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.
이렇게 한꺼번에 관리하니깐 하나하나 set함수를 쓰지 않아도 handleOnChange하나의 함수로도 변경할수 있네요! 더 깔끔한 코드인 것 같아 배워갑니다 !! ㅎㅎ
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.
저도 검색하다가 괜찮은 것 같아서 적용해 보았습니당 👨🏼💻
<div className="input-container"> | ||
<span className="input-title">만료일</span> | ||
<div className="input-box w-50" onChange={handleOnChange}> | ||
<input className="input-basic" type="text" name="expireMonth" placeholder="MM" maxLength="2" required /> |
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을 number이 아닌 text로 지정한 이유를 알고 싶습니다! handleOnChange에서 카드넘버만 정규식을 통해서 걸러지는 것 같은데 만료날짜가 text이면 이것도 숫자인지 아닌지 판별을 해주는 로직이 필요하지 않을까 생각했습니다. (제가 놓치고 있는 부분이 있으면 알려주세요 ㅎㅎ)
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.
console.log(e.target); | ||
console.log(e.target.value); | ||
console.log(e.target.maxLength); | ||
if (e.target.name.slice(0, -1) === 'cardSubNumber') { |
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.
card number 뿐만 아니라 input type이 text나 password인 것도 숫자인지 체크하는 로직이 필요해보입니다! 제가 코드에서 못찾은 것일수도 있어서 놓친 부분이 있다면 알려주시면 감사하겠습니다!
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.
정확한 지적이에요. 아직 유효성검사 부분이 제대로 구현이 되지 않았습니다. 😓
console.log(inputs); | ||
console.log(e.target.value); | ||
|
||
if (!nameArray.includes(e.target.name)) 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.
혹시 nameArray를 따로 지정하신 이유가 있을까요? nameArray에 해당하는 경우 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.
각 영역의 마지막 입력이 끝났을때 포커스 이동을 하기 위해서 임시로 만든 것이 었는데 유효성 검사와 상관없이 이동이 되기 때문에 아직 미완성이에요. return을 넣은 이유는 각영역의 중간 예를 들어 카드번호 넣는 구간의 마지막이 아닌 중간부분을 넣었을때는 포커스 이동이 되지 않도록 한 것이었습니다. 😸
안좋은 컨디션 중에서도 부족한 코드 잘 봐주셔서 감사합니다. 세팅부분은 검색한 글 그대로 따라해본 거라 잘못된 부분이 있을 확률이 높을 꺼에요. 남겨주신 리뷰 잘 읽고 보완하도록 하겠습니다. (특히 유효성 검사, useRef 등) 리뷰 필요할 때 언제든지 말해달라고 해주셔서 맘이 든든했습니다. 🥇 @rachel490 님도 새로운 도전 끝까지 잘 마무리하셔서 좋은 결과 있기를 바라겠습니다 !! |
Step1 - Component-Driven Development
📝 Requirements
필수 요구사항
Storybook
상호 작용 테스트재사용 가능한 Component
작성카드 추가
<
(뒤로가기) 버튼 클릭 시, 카드 목록 페이지로 이동한다.- [ ] 카드 번호입력 폼에 라벨을 보여준다.-
가 삽입된다.*
로 보여진다.- [ ] 만료일 입력 폼에 라벨을 보여준다.MM / YY
로 placeholder를 적용한다./
가 삽입된다.- [ ] 카드 보안코드 입력 폼에 라벨을 보여준다.*
으로 보여진다.- [ ] 카드 비밀번호 입력 폼에 라벨을 보여준다.*
으로 보여진다.- [ ] 카드 소유자 이름 입력 폼 라벨을 보여준다.심화 요구사항 (선택사항)
Storybook
단위 테스트궁금한 점
- 요구사항마다 있는 라벨을 보여준다의 의미를 정확히 모르겠습니다. 라벨태그를 사용하라는 말인지? 아니면 에러메시지 창을 만들라는 의미인지? 혹시 아시는 분 있으면 알려주세요느낀 점