Skip to content
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

[페이먼츠 미션 Step 2] 황펭(양이진) 미션 제출합니다. #270

Merged
merged 66 commits into from
May 5, 2023

Conversation

Leejin-Yang
Copy link

@Leejin-Yang Leejin-Yang commented Apr 30, 2023

안녕하세요 케빈! 주말 잘 보내셨나요?
저는 집에서 얼른 미션을 진행해보았습니다.!
이번 step2 미션도 잘 부탁드립니다 😊

배포주소

스토리북

이번에 추가된 기능은 다음과 같습니다.

  • 카드사 등록
  • 카드 이름 등록

진행 과정

CardFormContext

카드 폼의 정보들을 Context API를 사용하는 것으로 리팩터링 했습니다.
ValueAction 컨텍스트로 구분하여 ValueCardItemAction은 각 입력 필드에서 사용합니다.
CardInfoForm의 크기가 크고, CardItem이 폼 컴포넌트에 있는 것이 문맥상 맞지 않아 분리가 필요하다 생각했는데요..
컨텍스트를 분리하니 각각에 필요한 값에 맞게 분리할 수 있었습니다. 또한 각 입력 필드를 분리해 action을 받아오게 함으로 CardInfoForm은 submit 이벤트만 가질 수 있게 했습니다.

추가적으로 CardNameRegisterPage에서 입력이 완료된 값을 가져와 최종 CardItem을 사용할 수 있었습니다.
여러 컴포넌트에서 사용하는 값을 context로 묶어주고 각각에 필요한 값을 뽑아다 사용할 수 있어 이번 리팩터링을 통해 좋은 경험을 했습니다.!

CardCompanyModal

모달에 애니메이션을 주기 위해 CSS keyframes를 사용하였습니다.
닫히는 애니메이션은 모달이 unmount 되기 전 실행하기 위해 timeout을 걸어주었는데, 이는 useTimeout 커스텀 훅을 분리해 사용했습니다.!

Storybook

컴포넌트를 작성 후 스토리를 작성해보았는데요. 스토리를 작성하고 props를 조작해보니 타입이 맞지 않거나 레이아웃이 깨지는 경우가 있었습니다. 컴포넌트별로 빠르게 이러한 부분을 찾을 수 있어 좋은 경험을 했습니다. 아직 스토리북 문법은 익숙하지 않아 BrowserRouterProvider를 그대로 사용했는데 이 부분에 대한 방법이 있을까요?

Redirect

유효한 페이지로 접속하지 않은 경우 루트 경로로 이동시켰습니다.
그리고 카드 이름 등록 페이지에서 이전에 입력한 카드 정보가 없을 때 루트 경로로 이동시켰습니다.

궁금한 점

CardFormContext, CardInfoRequired

현재 두 컴포넌트는 pages/contexts에 위치하고 있습니다.

  • CardFormContext: CardFormProvider를 사용하는 페이지(CardInfoRegisterPage, CardNameRegisterPage)를 감싸주기 위해
  • CardInfoRequired: 완전한 카드 정보 입력되었을 때 접속 가능한 페이지(CardNameRegisterPage)를 감싸주기 위해

두 컴포넌트는 react-router-domOutlet을 사용하였습니다. 해당 컴포넌트를 구조상 어느 위치에 두어야 하는지 애매합니다..
page 컴포넌트는 아니기에 pages 폴더에 위치하는 것이 맞을까? 라는 생각이 들었고, UI 요소가 아니기에 components 폴더에 위치하는 것이 맞을까라는 생각이 들었습니다. 두 컴포넌트는 페이지가 가져야할 환경이라고 생각해 pages에 contexts 라는 폴더를 생성해 두었습니다. 이러한 컴포넌트는 어느 곳에 위치해야할지 케빈의 생각이 궁금합니다.!

@JeongBin0227 JeongBin0227 self-assigned this May 1, 2023
@JeongBin0227
Copy link

image

완벽한 인풋이 아닐때, 어느 인풋이 잘못되었는지 알려주지 않는데 `다음` 버튼이 노출되지 않는게 조금 불편한 로직인거 같아요

@JeongBin0227
Copy link

카드 바뀔때, 인터렉션있는게 멋지네요 👍👍👍

@JeongBin0227
Copy link

두 컴포넌트는 react-router-dom의 Outlet을 사용하였습니다. 해당 컴포넌트를 구조상 어느 위치에 두어야 하는지 애매합니다..
page 컴포넌트는 아니기에 pages 폴더에 위치하는 것이 맞을까? 라는 생각이 들었고, UI 요소가 아니기에 components 폴더에 위치하는 것이 맞을까라는 생각이 들었습니다. 두 컴포넌트는 페이지가 가져야할 환경이라고 생각해 pages에 contexts 라는 폴더를 생성해 두었습니다. 이러한 컴포넌트는 어느 곳에 위치해야할지 케빈의 생각이 궁금합니다.!

구조는 정답이 없기 때문에 본인이 정한 규칙을 가져가거나, 아니면 팀단위 개발일 경우 팀의 규칙을 따르면 됩니다.
제 경험상으로 보통 Context 컴포넌트는 해당 Context를 사용하는 컴포넌트와 함께 위치시키는 것이 일반적입니다.

그래서 CardFormContext 컴포넌트와 CardInfoRequired 컴포넌트를 사용하는 페이지 컴포넌트가 위치하는 pages 폴더 내에 contexts 폴더를 생성하여 함께 위치시키는 것이 좋습니다. 이렇게 하면, 해당 페이지에서 필요한 Context를 직접 import하여 사용할 수 있으며, 페이지 컴포넌트와 Context 컴포넌트가 서로 관련이 있다는걸 코드를 보는사람에게 쉽게 알려 줄 수 있을거 같아요

Copy link

@JeongBin0227 JeongBin0227 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 황펭!

전반적으로 코드가 가독성도 좋고, 네이밍 규칙도 일정하고, hook 활용도도 좋은거 같습니다! 👍
질문 주신 부분이나 추가적인 코멘트 남겨드렸으니 확인부탁드려요!
고생하셨습니다 👍 💯

to {
bottom: -300px;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

name: string;
company: CompanyName;
number: Pick<CardNumber, 'first' | 'second'>;
expiredDate: ExpiredDate;
owner?: string;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모든 타입이 다 export 되지 않고, 필요한 타입만 내보내는거 좋은 습관인거 같아요!

Comment on lines 27 to 32
<span className={styles.password}>
{'third' in number ? '﹒'.repeat(number.third.length) : '﹒﹒﹒﹒'}
</span>
<span className={styles.password}>
{'fourth' in number ? '﹒'.repeat(number.fourth.length) : '﹒﹒﹒﹒'}
</span>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 이해한 코드가 맞다면, 카드에 3번쨰 4번째는 시크릿 코드로 보여주는 곳인데,
이미 개발하실때 third 와 fourth 인지 아는데, 이렇게 또 따로 처리하신 이유가 궁금합니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

카드 리스트를 보여주는 HoldingCardsPage와 카드 정보를 입력하는 CardInfoRegisterPage에서 차이점이 있어 따로 처리를 해주었습니다. 저장된 정보에는 third, fourth 값이 없습니다.!

경우에 따라 third, fourth 정보가 없을 때 텍스트를 보여주고, 있다면 입력과 동시에 같이 렌더링 되도록 했습니다!

Comment on lines +37 to +38
{expiredDate.month}
{expiredDate.month.length === 2 && <span>/</span>}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분을 expiredDate.month 가 유효하다면 으로 수정해서 {expiredDate.month} 와 / 를 보여주는것으로 수정해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

입력과 동시에 만료일 유효성 검사를 해주고 싶은데 공통된 change 이벤트에서 특정 검사를 해주는데 어려움이 있네요 🥲
위에 작성한 내용과 해당 부분은 step3에 진행해보겠습니다..!

}

const CardNameForm = ({ registerCard }: Props) => {
const nameInputRef = useRef<HTMLInputElement>(null);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useRef 와 useState의 차이점을 공부해보시면 좋을거 같아요.
특히 클로져와 함께 연관시키면 더 좋겠네요!

</button>
<div className={styles.previousButton}>
<Button type="button" onClick={handlePreviousButtonClick}>
&lt;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

< 을 넣어주신 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

svg 대신 텍스트로 대체 했습니다.!

@@ -1,4 +1,4 @@
label {
.label {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스로 바꾸신이유가 있나요?

Copy link
Author

@Leejin-Yang Leejin-Yang May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스크린샷 2023-05-04 오후 6 30 19

스크린샷 2023-05-04 오후 6 32 51

만약 클래스가 아닌 요소에 스타일을 입혔을 때, 위의 사진과 같이 헤더가 있다면 스타일이 겹치더라구요.!

);
};

export default CardFormProvider;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

폼을 다루다보면 인풋을 엄청 많이 다루게 될텐데, 인풋이 추가 될때마다 위 로직이 더 복잡하게 될거같아요.
폼을 다루는 방법이나 라이브러리들을 한번 공부해보시면 좋을거같아요!

Copy link
Author

@Leejin-Yang Leejin-Yang May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상태의 key와 값이 객체인 경우 내부의 키를 받는 구조로 변경해보았습니다 😄

관련 커밋: 0640a98

!isValidExpiredDate(Number(month), Number(year)) ||
!isValidCardData
)
return <Navigate to="/" replace />;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace 의 역할은 무엇일까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

입력한 카드 정보가 필수인 경우 페이지 처리를 해주기 위해 작성했습니다.
접속을 못하게 하기 위해 replace 속성을 주어서 홈으로 이동(push)이 아닌 홈으로 페이지를 바꾸게 했습니다!

@Leejin-Yang
Copy link
Author

Leejin-Yang commented May 4, 2023

안녕하세요 케빈! 2단계 수정 후 다시 리뷰 요청 드립니다.
피드백 주신 내용을 바탕으로 많이 고민해볼 수 있는 시간을 가졌습니다.! 감사합니다 👍
특히 사용성에 대한 피드백 감사드립니다 😄

아래와 같이 리팩터링 진행했습니다.

  • 카드 정보 상태 변경 함수 최소화
  • 입력에 대한 메세지 렌더링
  • 기타 커스텀 훅으로 분리(useAutofocus, useFieldFilled)

배포주소

Copy link

@JeongBin0227 JeongBin0227 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 황펭!

반영해주신 부분 모두 확인하였습니다 👍
스텝2 하느라 고생많으셨습니다! 💯 스텝3에서 뵙겠습니다~ 남은 미션 화이팅!!

@JeongBin0227 JeongBin0227 merged commit ecf06b5 into woowacourse:leejin-yang May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants