[1단계 - 페이먼츠 미션] 크리스(이송원) 미션 제출합니다#29
Conversation
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: SONG WON LEE <swon3210@users.noreply.github.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
HyeonaKwon
left a comment
There was a problem hiding this comment.
안녕하세요 크리스님! 리뷰 많이 늦어 정말 죄송합니다😭
세부 피드백은 코멘트로 확인해주시구 아래는 전체적인 피드백 말씀드릴게요~
- 공통 컴포넌트를 전체적으로 사용하고 있지는 않은 것 같아요. 만들었으면 사용을 해야겠죠~? label 과 input 을 공통 컴포넌트로 바꿔보면 좋을 것 같습니다!
- setCardStateByKey 를 모든 hook 에서 한번더 래핑할 정도로 useCallback 을 사용할 필요가 있었나 싶긴합니다. 그래도 각 hook 이 정형화되어 있어서 코드 읽기는 편하네요! 그리고 질문주신 내용은 어차피 import 해서 사용할 때마다 새로운 값으로 저장될 것이기 때문에 문제는 없어보입니다! 그러려고 hook 을 쓰는거니까요.
- 이 정도의 미디어쿼리 양은 전혀 영향을 주지는 않겠지만, 모을 수 있다면 모아두는게 좋을 것 같네요! 큰 문제는 없을 것 같습니다.
| ...state, | ||
| [cardStateKey]: cardStateValue, | ||
| })); | ||
| }; |
| <Route> | ||
| <CardListPage cardListState={cardListState} /> | ||
| </Route> | ||
| </Switch> |
There was a problem hiding this comment.
<Route path="/photo" component={컴포넌트이름}/>요런 형식도 고민해봐주세요!
There was a problem hiding this comment.
이런 형식으로 작성하면 prop 넘겨주는게 힘들다고 판단해서 지금의 형식을 택했습니다. 저 형식으로도 prop 을 넘겨주는 방법이 있을까요?
There was a problem hiding this comment.
아하 넵! 넘기려면 함수의 return 값 형태로 넘길 수 있겠죠?!
| maxLength="4" | ||
| className={cx("card__number-input", "card__number-input--visible")} | ||
| defaultValue={firstCardNumber} | ||
| disabled |
There was a problem hiding this comment.
이 부분은 이용자가 입력해서 바꿀 수 없게 만들고 싶었기 때문입니다. 카드 번호를 보여줄 때, 뒤 8 글자는 점으로 숨겨서 보여줘야 했기 때문에 따로 점을 보여주는 컴포넌트를 만들기보다는 input 의 타입을 password 로 해서 그냥 그렇게 해서 나타나는 점들을 이용하고 싶었었고, 뒤 8글자를 input 태그로 썼기 때문에 일관성을 지켜주고 싶어서 앞의 8글자도 input 태그로 만들고 diabled 속성을 준 것입니다.
그러나 지금은 이게 좋지 않은 방법이라는 것을 알게 되었습니다. 다른 브라우저나 OS 에서 접속했을 때 해당 부분이 많이 깨지는게 보이는군요. 고쳐서 반영하도록 하겠습니다!
| </div> | ||
| <div className={cx("card__bottom")}> | ||
| <span className={cx("card__owner")}>{cardOwner}</span> | ||
| <span className={cx("card__expiration")}>{Object.values(cardExpiration).every(value => value !== "") && Object.values(cardExpiration).join(" / ")}</span> |
There was a problem hiding this comment.
요런거는 위에서 변수로 선언하는게 좋을 것 같아요! 어떤 의민지 코드를 다 읽어봐야하니까요!
| className={cx("card-expiration-input__input")} | ||
| maxLength={2} | ||
| onChange={onInputChange} | ||
| placeholder={yearPlaceholder} |
There was a problem hiding this comment.
공통 컴포넌트가 아닌 구체적인 컴포넌트인데도 플레이스 홀더를 props로 받는 이유가 있을까요?
There was a problem hiding this comment.
제가 이전에는 공통 컴포넌트와 구체적인 컴포넌트의 차이를 완전히 견지하지 못하고 있었기 때문에 이 부분에 대한 충분한 고민이 부족했습니다. CardExpirationInput 이라는 컴포넌트 자체를 지우고 기존의 컴포넌트 중 SeperatedInputList 컴포넌트가 역할을 대신하도록 만들었습니다.
이렇게 하면 placeholder를 받아도 무리가 없을까요?
| moveAnimation: "move-down", | ||
| })); | ||
| }, 350); | ||
| }, [setState]); |
There was a problem hiding this comment.
setToggled, setUntoggled 둘 다 두번째 인자에 setState 를 넣는 이유가 있나요??
There was a problem hiding this comment.
CRA 의 린터가 내부의 로직이 setState 함수에 의존하고 있기 때문에 dependency 에 setState 를 추가해야 한다고 생각했기 때문입니다. 그런데 setState 는 결국 리액트에서 컴포넌트가 unmount 되지만 않는다면 매번 새로 함수를 생성하지 않기 때문에 굳이 이러한 처리를 할 필요가 없는 것이라는 말씀이시군요!
디펜던시 비우고 린터가 이 부분 지적하지 않도록 주석 넣어주도록 하겠습니다!
| }; | ||
| }; | ||
|
|
||
| export default useToggle; |
| import cardInfo from "../data/banks.json"; | ||
|
|
||
| const preprocessing = (cardNumber) => { | ||
| return cardNumber.replace(/[^0-9]/g, "").slice(0, 6); |
| (card) => card.name === cardCompany | ||
| ); | ||
|
|
||
| return card?.color; |
There was a problem hiding this comment.
여기서 card 가 없다면 undefined 가 반환되는 것 같은데, 25라인처럼 default 컬러를 반환해줄 필요는 없을까요??
There was a problem hiding this comment.
예외처리에 대한 꼼꼼함이 아직 부족하다는 것을 많이 느끼게 되네요...말씀하신대로 반영하도록 하겠습니다!
| } | ||
| if (numberText instanceof Array) { | ||
| return numberText.every((value) => value.length === length); | ||
| } |
There was a problem hiding this comment.
파라미터 명도 numberText 로 string 이겠지? 생각이 들게 하는 변수명인데, 실제 내부에서는 객체 또는 배열로 받고 있는게 어색해보입니다.
그리고 근본적으로 numberText 가 여러 타입인 이유는 무엇일까요?
There was a problem hiding this comment.
해당 코드가 쓰인 isAllNumberListTextLengthCorrect 라는 함수가 object 도, list 도 받아서 처리할 수 있는 '편리한' 함수로 만들고 싶었기 때문입니다. 하지만 지금은 이게 성급한 추상화라는 생각이 드네요. 나름 이름을 일반화 해보려고 numberText 라는 이름을 쓴 것이지만 오히려 혼란만 키운 것 같습니다. 당시 급하게 내느라 신경을 많이 쓰지 못한 부분이 보이는 것 같습니다.
해당 함수를
isAllTextFilledInList
isAllTextFilledInObject
로 나누어 리팩토링 했습니다.
HyeonaKwon
left a comment
There was a problem hiding this comment.
isAllNumberTextLengthCorrect 를 checkAllValueLength() 이런식으로 범용적으로 바꿨어도 좋았을 것 같네요! :)
수정사항 잘 봤습니다. 반영해주셔서 감사해요! 👍 고생하셨습니다.
데모 페이지
URL : https://keen-wright-2fa546.netlify.app
미리보기
기타 링크
피그마 : https://www.figma.com/file/6jB9o86LCjv7skopVOPfTH/payments-Chris-ZZZoomo?node-id=0%3A1
스토리북 : https://upbeat-carson-a9553c.netlify.app/?path=/story/payments-navigationbutton--primary
컴포넌트 구분 시각화
프로젝트 구조도
📝 Requirements
컴포넌트 구조 확립 및 퍼블리싱
Storybook단위 테스트카드 입력 페이지
카드 추가 완료 페이지
카드 목록 페이지
디렉터리 구조도
궁금한 점
대부분의 비즈니스 로직을 저는 커스텀 훅을 통해 관리하고 있습니다. 이렇게 하면 로직 자체를 재사용할 수 있는 기회가 늘어나리라 생각했기 때문입니다. 그러나 이렇게 하면 어떤 컴포넌트든 import 를 통해 해당 도메인의 로직을 수행해버릴 수 있는 위험도가 늘어날 수 있을까요? 제가 실제로 커스텀 훅을 적절히 사용하고 있는지가 궁금합니다.
지금은 시간 상의 문제로 인해 아주 기본적인 모바일 - 타블렛 - 데스크탑의 반응형 디자인이 구현되어 있습니다. 하지만 이를 위해 미디어 쿼리를 중복해서 실행하고 있는 부분이 간간히 있는데요, 지금 당장은 사용되고 있는 미디어 쿼리의 수 자체가 적어서 눈에 띄는 성능상으 이슈는 없지만, 이러한 미디어 쿼리의 중복 실행이 성능에 어느 정도 유의미한 영향을 줄 수 있는지가 궁금합니다.
알려드릴 점