[클린코드 리액트 2기 우경준] 페이먼츠 미션 Step 3 #98
Conversation
|
안녕하세요. 준일님! 다시 살펴보니 정말 보기 어려운 코드였는데, 😭 성심성의껏 리뷰해주셔서 진심으로 감사합니다. 🙏 2023-03-29.4.04.12.mov변경점객체의 책임을 적절하게 분산하고 추상화 레이어를 줄여 설계를 최대한 알기 쉽도록 변경했습니다. (말씀 주신 의존관계 다이어그램이 개선에 큰 도움이 되었습니다! 👍) 피그잼 링크 개선된 설계에 대한 준일님의 의견이 궁금합니다! 이외 UX면에서 리뷰주신 AutoFocus 또한 변경했습니다. card store 설계 변경객체 생성에 필요한 추상화 레이어를 줄였습니다.
input model의 객체 역할을 컴포넌트에 분담"입력되어야 하는지 말아야하는지"에 대한 판단을 객체 메소드를 통해 하고 있었습니다. 이 메소드는 컴포넌트의 움직임에 대한 정보로 객체가 가지기엔 부적절하다고 판단했습니다. 데이터 구조 통일store의 property 모두 Array로 통일하고 불필요한 Type 검사를 줄였습니다. 이전에 Array와 Array가 아닌 것이 혼재되어 있어, store를 다룰 땐, 항상 Array Type인지 아닌지에 대한 검사를 행했었습니다. 외부 storage와의 통신 설계 변경불필요한 추상화를 줄이고 hook -> service -> storage 구조로 다시 설계해보았습니다.
service 객체useFetch 재사용을 위해 service 객체를 주입해 사용할 수 있도록 설계했습니다. Error 표현 픽스전체 Input 중에 단 하나 밖에 표현안되던 Error 표현을 해결했습니다.이제 invalid한 Input 모두에 에러가 표시됩니다. Active 추가 된 점말씀주신 UX 향상을 위한 몇가지 feature가 적용되면 좋겠다고 생각해서 적용해보았습니다. 만기년도가 현재 년도 이후이어야 함.Expire Year는 이제 현재 년도 이후 년도만 입력할 수 있습니다. (의견) 년월 입력은 Date select 혹은 select box로 바꾸는 것이 좋을 것 같습니다.직접 Number를 입력하는 것에는 valid 체크에 한계가 있고 사용자 입력면에서도 불편하다고 생각되어 Date select 혹은 select box로 입력이 가능한 숫자만 제공하는 것이 UX에서도 유지보수 면에서도 좋은 것 같습니다. cardCreate페이지 진입시 AutoFocuscardCreate 페이지 진입했을 때, AutoFocus 및 invalid 체크가 되도록하여 사용자가 바로 입력할 수 있도록 했습니다. (UX 향상) Application Context 추가라이브러리 배포시에 라이브러리 사용자에게서 받은 것을 App안에서 이용하기 위한 Context입니다. Exception 처리라이브러리 사용자의 input을 확인하는 Exception 처리를 추가했습니다. 카드 리스트에서 카드 클릭시 modal이 나오도록 변경모바일 대응으로 �화면이 넘어가는 interaction이 아닌 카드 상세 modal이 나오도록했습니다. 터치 실수를 염두해서 만들었습니다. 기존의 interaction은 modal에 있는 버튼에 설정됩니다. 2023-03-29.11.29.32.mov감사합니다. |
There was a problem hiding this comment.
안녕하세요 경준님!
리뷰가 많이 늦었네요 ㅠㅠ 죄송합니다 🙏
일단 복잡한 구조를 개선하려고 애쓰신게 보이네요 ㅎㅎ 고생하셨습니다 👍👍
전체적인 설계에 대한 리뷰를 드리자면,
CardModelClass (이지만 DOM 종속적인..) -> state -> store -> context -> hooks -> components
이런 모습인데요, model이 dom에 종속적인 모습이 적절한가 고민해본다면 그건 아닌 것 같아요.
최소한 context 에서 부터 DOM에 종속적이여야 하지 않을까요?
가령 ssr에 대해 고민해본다면 store는 server에서 정의해서 건내줄 수도 있거든요.
그런데 server가 dom에 종속적이면 당연히 오류가 발생하게 되겠죠?
그리고 ApplicationContext의 경우 service를 주입받아서 쓰고 있네요..! 리뷰할때는 몰랐는데 지금 생각해보니 외부에서(아예 다른 패키지에서) 이걸 주입해서 쓰고싶어서 그렇게 하신 것 같다는 생각이 들었어요 ㅎㅎ
그런데 package로 받아서 사용할 때는 service를 주입하는게 아니라 그냥 함수(addCard, deleteCard, updateCard 같은)를 주입해서 써야하지 않을까요?
service를 정의해서 사용하는 과정도 굉장히 복잡하다고 느껴졌어요.
localStorageService -> ApplicationContext -> useFetch -> useFetchCardList
이런 단계인데, 여기서 useFetchCardList는 결국에 함수를 반환하고 있기 때문에, useFetchCardList가 진짜 서비스의 역할을 하는게 아닌가 싶었습니다.
결국 위에서 정의한건, storage -> cardService 이렇게 두 단계로 요약할 수 있는게 아닌가 싶고, cardService의 모습이 fetchCard postCard deleteCard 같은 메소드를 가지고 있으면 되지 않을까 싶습니다.
이 외에도 꽤 많은 코멘트를 남겼는데 차근차근 확인해보시면 될 것 같습니다!
Step3가 무척 길어진 상태라서, 이번 단계는 여기서 마무리하도록 하겠습니다 ㅎㅎ
이미 장바구니 미션 진행중이신 것 같아서, 마지막 미션 진행하실 때 참고하시면 될 것 같아요!
생각보다 더 긴 호흡을 함께했네요! 같이할 수 있어서 즐거웠습니다 경준님 😁
끝까지 잘 마무리하기를 기대하겠습니다! 화이팅 💪💪
| @@ -1,3 +1,7 @@ | |||
| import '../src/styles/index.css'; | |||
| import * as jest from 'jest-mock'; | |||
| window.jest = jest; | |||
| cardStore.some((cardStateList) => | ||
| cardStateList.some((cardState) => { | ||
| const isActiveElement = cardState.ref === document.activeElement; | ||
| if (isActiveElement) activeState = cardState; | ||
| return isActiveElement; | ||
| }) | ||
| ); |
| } | ||
|
|
||
| export function createArray<T>(arrayLength: number, callback: (el: T, index: number) => T): T[] { | ||
| return new Array(arrayLength).fill(1).map(callback); |
| export function entryObject<T extends { [key: string | number | symbol]: unknown }>(obj: T): [keyof T, T[keyof T]][] { | ||
| return Object.entries(obj) as [keyof T, T[keyof T]][]; |
There was a problem hiding this comment.
타입 단언을 해주고 있기 때문에, 이렇게 return type은 정의하지 않아도 무방할 것 같아요!
export function entryObject<T extends { [key: string | number | symbol]: unknown }>(obj: T) {
return Object.entries(obj) as [keyof T, T[keyof T]][];|
|
||
| cardStore.some((cardStateList) => | ||
| cardStateList.some((cardState) => { | ||
| const isActiveElement = cardState.ref === document.activeElement; |
There was a problem hiding this comment.
activeElement는 처음 보네요 ㅎㅎ 덕분에 알아갑니다 👍
| } | ||
|
|
||
| const CardCompanySelector = memo(({ onCardCompanyClick }: CardCompanySelectorProps) => { | ||
| export const CardCompanySelector = memo(function CardCompanySelector({ onCardCompanyClick }: CardCompanySelectorProps) { |
There was a problem hiding this comment.
hooks 내부에 이런 component 파일이 위치해있는게 어색하네요..! 아예 밖으로 빼면 어떨까요?
| } | ||
|
|
||
| function CardCompany({ cardCompany, onClick }: CardCompanyProps) { | ||
| export function CardCompany({ cardCompany, onClick }: CardCompanyProps) { |
| useEffect(() => { | ||
| if (!cardStore || hasRun.current) return; | ||
|
|
||
| findInvalidStoreAndFocus(cardStore); |
There was a problem hiding this comment.
이 함수는 사이드 이펙트가 존재하는 함수인데,
이게 utils라고 볼 수 있을까요?
| <ExpireDatesInputList expireDates={expireDates} /> | ||
| <CardOwnerInput cardOwners={cardOwners} /> | ||
| <SecurityCodesInputList securityCodes={securityCodes} /> | ||
| <PasswordsInputList passwords={passwords} /> | ||
|
|
||
| <SubmitButton /> |
There was a problem hiding this comment.
Submit에 대한 동작을 Creator에서 정의해줘야 하지 않을까요?
| export function useCardSubmitEvent() { | ||
| const cardStore = useCardContext(); | ||
|
|
||
| // select된 inputState가 변하면 아래 함수도 새로 만들어져야 하므로, useCallback은 적용하지 않음. | ||
| return (e: MouseEvent<HTMLElement>) => { | ||
| if (!cardStore) return; | ||
|
|
||
| const { cardCompanies, cardNumbers, expireDates, cardOwners, passwords, securityCodes } = cardStore; | ||
| const invalidState = findInvalidStoreAndFocus([ | ||
| cardNumbers, | ||
| expireDates, | ||
| cardOwners, | ||
| passwords, | ||
| securityCodes, | ||
| cardCompanies, | ||
| ]); | ||
| if (invalidState) { | ||
| e.preventDefault(); | ||
| invalidState.ref?.focus(); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
현재 구조에서는 이 hook이 button에 종속적인데, 그래도 괜찮을까요?
There was a problem hiding this comment.
MouseEvent에는 종석적이지만, button에 종속적이진 않다고 생각됩니다 ㅎㅎ
혹시 어떤 부분때문에 button에 종속적이라고 판단하셨는지 여쭤봐도될까요?
There was a problem hiding this comment.
이름이 submit을 처리하는건데, 실제로는 mouse event를 받아서 처리하는 구조라서요!
그리고 hook이 SubmitButton 내부에 있기 때문에 이건 SubmitButton 내부에서만 쓰이는건가? 라는 생각이 든답니다 ㅎㅎ
이런 로직은 Button이 아니라 Form에 종속적이여야 하지 않을까요?
There was a problem hiding this comment.
아! 그렇군요! 순수하게 submit을 위한 로직 뿐만아니라 eventListener가 통째로 구현되어 있어서 그렇군요!
말씀처럼 Element가 아닌 Form에 종속적으로 하는 것이 맞는 것 같습니다!
감사합니다. 😁
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
안녕하세요 준일님!
페이먼츠 마지막 step 잘 부탁드리겠습니다!
PR 후에도 피드백 주시거나 필요한 부분이 있다면 계속해서 추가해나갈 예정입니다.
실행 방법
스토리북 배포 URL
주요 변경 사항
requirements
유효성 검증
유효성 검증 실패에 대한 UI/UX 추가
유효한 값 입력시 다음 필드로 Input Focusing
카드
카드 번호 앞 8자리로 카드사를 추정하여 그 테마를 카드 UI에 반영한다.
카드사를 선택하지 않아도 모달을 닫을 수 있다.
카드사가 선택되고 유효한 카드 번호 16자리를 모두 입력하면, 자동으로 만료일로 focus된다.
별칭 수정 가능
보안코드 툴팁
클릭 시, 보안코드 관련 안내 메시지를 보여준다.
focusout시, 툴팁이 닫힌다.
가상 키보드
[] 마스킹 처리된 값 입력시 사용
[] 숫자를 랜덤으로 배열
질문
�Bottom-up 컴포넌트 개발 방식을 통한 재사용 가능한 추상화된 컴포넌트를 만드는 것이 이번 TDD 과정의 목표였습니다. 노력했지만, 잘 적용됐는지 궁금합니다.
함수와 hook의 역할이 �적절하게 나뉘었을까요?
책임이 명확하고 추상화된 함수부터 조금씩 구체화하면서 비즈니스 로직을 구체화하도록 노력했습니다.
Application의 전체적인 �설계는 어떻게 생각하시는지 궁금합니다.
각 input마다 객체를 두고 자신의 상태를 스스로 판단해서 결과를 내도록 했습니다.