Skip to content

[1단계 - 페이먼츠] 파슬리(김윤아) 미션 제출합니다.#331

Merged
moonheekim0118 merged 67 commits into
woowacourse:anttieyfrom
anttiey:step1
Apr 23, 2024
Merged

[1단계 - 페이먼츠] 파슬리(김윤아) 미션 제출합니다.#331
moonheekim0118 merged 67 commits into
woowacourse:anttieyfrom
anttiey:step1

Conversation

@anttiey
Copy link
Copy Markdown
Member

@anttiey anttiey commented Apr 18, 2024

안녕하세요, 호프! 저는 파슬리라고 합니다 🌱 1단계 미션 구현을 완료하게 되어 PR 남깁니다 ☺️

👩🏻‍💻 개요

첫 번째 미션 페이먼츠 1단계 Component & Storybook 코드입니다.

배포 페이지

배포 페이지

스토리북 배포 페이지

실행 방법

npm install

// 로컬 환경 실행
npm run dev

// 스토리북 실행
npm run storybook

📚 기능 요구 사항 & 해결 과정

기능 요구 사항

  • 재사용 가능한 Input Component를 개발한다.
  • 카드 정보를 효과적으로 렌더링 하기 위한 상태 관리를 경험한다.
  • Storybook을 사용하여 컴포넌트의 다양한 상태를 시각적으로 테스트한다.

1. 컴포넌트 구조

스크린샷 2024-04-18 오후 1 01 10

반복되는 컴포넌트와 스타일을 분리하고 재사용 가능한 컴포넌트를 만들기 위해 페어와 상의를 한 결과, 저희는 다음과 같은 기준으로 컴포넌트를 구성하기로 결정했습니다.

  • 공통 컴포넌트 (사진에서 왼쪽)

    • Input, InputField, TitleContainer
    • 도메인 로직을 포함하지 않는다.
    • 두 개 이상의 컴포넌트에서 공통적으로 사용한다.
  • 비즈니스 컴포넌트 (사진에서 오른쪽)

    • CardExpirationInput, CardNumberInput, CardOwnerInput, CardPreview
    • 도메인 로직을 포함한다.
    • 공통 컴포넌트를 필요한 위치에 적절하게 확장하여 사용한다.

2. 카드 정보 상태 관리

  • 최상단 컴포넌트인 App 에서 모든 상태를 관리하고 각각의 컴포넌트에 props 로 상태를 전달합니다.
  • 아직은 2 depth 이상 props 를 전달하고 있지 않으나 추후 기능이 발전한다면 props drilling 문제가 발생할 수 있을 것 같습니다.

3. 스타일링

  • styled-components 를 사용했습니다.
  • S-dot 네이밍을 활용해 스타일 코드와 컴포넌트 코드를 구분했습니다.
  • 컴포넌트 파일명이 component.tsx 라면 스타일 파일명은 component.style.ts 로 지정해 /styles 폴더 안에 작성했습니다.

4. Storybook

  • 모든 컴포넌트를 시각적으로 확인해 볼 수 있습니다.
  • 스토리북의 autodocs 를 활용해 기본 문서를 자동적으로 생성하도록 구현했습니다.
  • 스토리북의 argTypes 를 활용해 컨트롤 패널에서 다양한 옵션을 확인할 수 있도록 구현했습니다.

추가 질문은 코멘트로 남겨 놓도록 하겠습니다! 잘 부탁드립니다 ☺️

anttiey and others added 30 commits April 16, 2024 14:36
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Co-authored-by: Seongjin Hong <hongseongjin.to@gmail.com>
Comment thread src/components/CardExpirationInput.tsx Outdated
type="text"
placeholder="MM"
maxLength={CARD_EXPIRATION.MAX_LENGTH}
onChange={onMonthChange}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Q.
사용자 입력에 대한 유효성을 즉각적으로 판단하고 피드백을 제공하기 위해 onChange 를 사용하고 있습니다. 따라서 사용자 경험 측면에서는 좋다고 생각되지만 입력이 변경될 때마다 state가 변화되면서 불필요한 리렌더링이 발생할 수 있겠다는 생각을 했습니다. 이런 관점에서 state 변경 횟수를 줄이면서도 효과적인 피드백을 제공할 수 있는 개선 방향이 있을까요?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

좋은 고민을 해주셨네요ㅎㅎ 아마 제어컴포넌트와 비제어컴포넌트에 대해서 알아보시면 도움 되실 것 같아요 :)

@moonheekim0118 moonheekim0118 self-requested a review April 18, 2024 08:27
@moonheekim0118 moonheekim0118 self-assigned this Apr 18, 2024
Copy link
Copy Markdown

@moonheekim0118 moonheekim0118 left a comment

Choose a reason for hiding this comment

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

안녕하세요 파슬리 🌱 만나서 반가워요 :) 짧은 시간동안 구현해주시느라 고생많으셨어요.
저도 이번 미션동안 잘부탁드립니다ㅎㅎ
일단 학습 목표를 굉장히 잘 충족해주신 것 같아요.
재사용 가능한 컴포넌트와, 도메인 로직이 섞인 컴포넌트를 파슬리만의 기준대로 잘 분리해주신 것 같아서 인상깊었어요. :)

그 외에 전체 코드에 해당하는 피드백은 아래와 같아요.

폴더구조 관련

현재 폴더구조를 보면 코드의 기능별로 (components, styles, stories) 파일을 분리해주신 것 같아요.
이러다보니까 어떤 컴포넌트에 어떤 스타일이 적용되고, 스토리북 테스트 작성 여부 파악하기 조금 헷갈렸어요. 😅

  1. 이렇게되면 코드의 응집도가 낮아지는 문제점이 있을 것 같은데요, (같은 목적의 코드가 프로젝트 전반에 다 흩어져있게 되어서요) 한번 어떤식으로 개선해봐주실지 고민해주시면 좋을 것 같아요 :)
  2. 컴포넌트 내부에서도 파슬리가 세운 기준대로 (공용 컴포넌트 / 도메인 컴포넌트) 폴더를 구분해볼 수 있을 것 같은데, 어떻게 생각하시나요~? 지금 구조상으로는 각각이 어떤 종류의 컴포넌트인지 파악하기 어려워서요ㅎㅎ

이번 기회에 현재 파슬리의 프로젝트에 적당한 폴더구조에 대해서 한번 고민해봐주시면 좋을 것 같아요 🙂 (정답은 없습니다ㅎㅎㅎ)

그러면 고생하셨어요! 리뷰 반영후 다시 불러주세요 :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

오 깃헙 액션까지 활용하셨군요 👍 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

이번 미션은 배포해야 할 페이지가 두 개가 되어 (기본 웹 페이지, 스토리북 페이지) Github Actions 를 사용해 배포를 자동화하면 좋겠다고 생각했습니다 ☺️

Comment thread src/App.tsx Outdated
import CardOwnerInput from './components/CardOwnerInput';
import CardPreviewBox from './components/CardPreview';

import './styles/reset.css';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

기왕 styled component 를 사용하신 김에 GlobalStyle 을 활용해보는것도 좋겠네요ㅎㅎ

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

GlobalStyles 를 활용해 reset.css 와 같은 역할을 하도록 설정할 수 있었군요! 이와 동시에 자주 쓰이는 color font-size 등을 한 곳으로 모아서 관리해 보면 좋겠다는 생각이 들어서 GlobalStylesThemeProvider 를 추가해 보았습니다 🙌🏻

fd008ee e63af28

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

와~ 너무 좋습니다 👍 👍

Comment thread src/App.tsx Outdated

function App() {
const [cardNumber, setCardNumber] = useState<string[]>(['', '', '', '']);
const [month, setMonth] = useState<string>('');
Copy link
Copy Markdown

@moonheekim0118 moonheekim0118 Apr 18, 2024

Choose a reason for hiding this comment

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

useState에 기본값을 주입하게 되면 해당 값을 사용하여 자동으로 타입추론을 하기 때문에 굳이 타입을 선언하지 않아도 됩니다ㅎㅎ (타입추론을 잘 활용하는 것도 타입스크립트를 현명하게 사용하는 방법입니다 :)

Suggested change
const [month, setMonth] = useState<string>('');
const [month, setMonth] = useState('');

Copy link
Copy Markdown
Member Author

@anttiey anttiey Apr 22, 2024

Choose a reason for hiding this comment

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

호프의 리뷰를 통해 타입 추론 활용하는 방법에 대해서 생각해 보게 되었습니다!

타입스크립트가 해줄 수 있는 일을 굳이 개발자가 직접 할 필요가 없다는 말이 크게 와닿았습니다. 기본적으로 최대한 타입 추론을 활용하여 코드를 작성하는 것이 좋다고 생각됩니다. 다만, 코드를 읽는 사람의 입장에서 복잡한 타입의 경우에는 타입을 명시해 주는 게 도움이 되지 않을까 생각했습니다.

결론적으로,

  • 명확하게 타입을 파악할 수 있는 경우 -> 타입을 명시하지 않아도 가독성에 큰 영향이 없으므로 타입 추론을 활용한다.
  • 복잡한 타입, 여러 타입이 올 수 있는 경우 -> 타입을 명시한다.

라는 기준을 세워 보았습니다. 이러한 기준에 대해서 호프는 어떻게 생각하시는지 궁금합니다 🧐

이번 미션은 단번에 타입을 알 수 있는 경우가 대부분이므로 불필요하게 선언된 타입을 제거해 보았습니다.
8ef32c2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

저 역시도 파슬리의 의견에 동의해요!

추가적으로 만약에 useState에 복잡한 타입이 올 경우 타입추론이 잘 되지 않을 수도 있습니다ㅎㅎ
그럴 때는 타입을 명시하는게 맞겠죠!

좋은 고민 해주시고 공유해주셔서 감사해요 :)

Comment thread src/App.tsx Outdated
Comment on lines +18 to +20
<S.CardPreviewBox>
<CardPreviewBox cardNumber={cardNumber} month={month} year={year} owner={owner} />
</S.CardPreviewBox>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

컴포넌트 네이밍과 styled componenet 네이밍이 동일하니, 조금 헷갈릴 것 같네요. 😅
특정 컴포넌트를 스타일 용도로 감싸는 태그 네이밍은 주로 -wrapper 혹은 -container 등이 무난히 사용되는 것 같아요.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

확실히 컴포넌트 네이밍과 스타일 네이밍은 다르면 좋겠다는 생각이 드네요! CardPreviewBoxWrapper 로 수정해 보았습니다 🙌🏻

071b030

Comment thread src/App.tsx Outdated
Comment on lines +22 to +24
<CardNumberInput setCardNumber={setCardNumber} />
<CardExpirationInput setMonth={setMonth} setYear={setYear} />
<CardOwnerInput setOwner={setOwner} />
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

useState 의 seState 함수를 props 로 직접 주입하고 계시군요?
이렇게하면 부모 컴포넌트에 속해있는 state 에 대한 제어권을 통으로 자식 컴포넌트에게 주는 셈인데요,
한번 관련된 키워드로 찾아보시고, 어떤 점이 문제일지 고민해봐주신 후 개선해봐주시면 좋을 것 같아요 😊

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

setState 함수를 직접 주입하는 경우 자식 컴포넌트는 부모 컴포넌트에 매우 종속적이게 되고, 자식 컴포넌트에서 setState 함수를 호출할 경우 업데이트 되는 컴포넌트가 아닌 곳에서 비동기 함수가 호출되기 때문에 예측이 불가능한 코드가 될 가능성이 있다는 사실을 알게 되었습니다.

부모 컴포넌트에서 state 를 업데이트 하도록 하는 것이 좋은 코드 컨벤션을 유지하는 방법이며, 부모 컴포넌트에서 handler 함수를 생성하여 넘기는 방식이 대표적이라고 하여 적용해 보았습니다!

1d11de2

Comment thread src/components/InputField.tsx Outdated
<S.InputField>
<S.InputLabel>{label}</S.InputLabel>
<S.InputContainer $length={length}>{children}</S.InputContainer>
<S.ErrorMessage>{errorMessage}</S.ErrorMessage>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

에러메시지가 없을 때에도 요 태그가 노출되어서 왜인가 했더니 height 를 고정시켜주시기 위해서인 것 같아요ㅎㅎ

그런데 글자 태그의 height 이 폰트사이즈에 의해서가 아니라 임의로 부여된 px값으로 고정되는건 일단 유지보수에 좋지 못할 것 같아요. 추후에 폰트사이즈가 변경되면 px 값도 변경되어야 하니까요?

또한, 요 InputField 컴포넌트에 errorMessage 와 함께 help text 가 하단에 추가되어야한다면 어떨까요? (대체로 help text와 error message는 동시에 노출되지 못함)

이런 경우 저는 주로 position absolute 등을 사용해서 전체 컴포넌트의 높이값에 영향을 주지 않도록 하는 편입니다ㅎㅎ 에러메시지 태그는 실제 에러메시지가 prop으로 주입받았을 때만 렌더링하도록 하고요 :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

errorMessage 가 존재할 때만 렌더링하도록 변경하고 CSS position 속성을 활용해 배치를 수정해 보았습니다.

d27c252

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 👍 👍

Comment thread src/constants/Message.ts Outdated
Comment on lines +2 to +6
INVALID_EXPIRATION_MONTH_LENGTH: '유효 기간의 월은 01 ~ 12 사이의 두 자리 숫자여야 합니다.',
INVALID_EXPIRATION_YEAR_LENGTH: '유효 기간의 년도는 두 자리 숫자여야 합니다.',
INVALID_CARD_NUMBER_LENGTH: '카드 번호는 4자리 숫자여야 합니다.',
INVALID_CARD_OWNER_CHARACTER: '소유자 이름은 영어 대소문자로 구성되어야 합니다.',
INVALID_CARD_OWNER_LENGTH: '소유자 이름은 30자 이하여야 합니다.',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

해당 에러메시지 내부의 숫자들은 위의 상수를 활용하면 더 좋겠네요 😊

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Condition 파일의 상수를 활용하여 에러 메시지 내부의 숫자를 대체해 보았습니다.

ff06452

export const Default: Story = {
args: {
isValid: true,
type: 'text',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

현재 스토리북의 컨트롤 탭에서 보면 요 타입을 사용자가 직접 string 으로 입력해야하는데요,
스크린샷 2024-04-18 오후 6 47 51

Input의 인터페이스를 잘 모르는 사람은 요 type 에 어떤 string 이 들어가야하는지 모를 것 같아요 🤧

스토리북의 controls 기능을 사용해서 스토리북 사용자 경험을 더 높여볼 수 있을 것 같아요 :)
https://storybook.js.org/docs/essentials/controls

Copy link
Copy Markdown
Member Author

@anttiey anttiey Apr 22, 2024

Choose a reason for hiding this comment

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

Input 에도 controls 기능을 활용할 생각을 못했네요! 간단하게 이번 미션에서 사용되는 text number 정도의 옵션을 넣어 보았습니다 🙌🏻

a1a145a

Comment on lines +14 to +18
args: {
setMonth: () => {},
setYear: () => {},
},
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이런 argument들을 사용자가 직접 바꿔야할 필요가 있을까요? 개발에서만 사용이 되고, 실제 스토리북 테스트에서는 별다른 역할을 하지 않는 argument 들은 사용자가 조작하지 못하도록 막을 방법은 없을까요?

스크린샷 2024-04-18 오후 6 53 23

Copy link
Copy Markdown
Member Author

@anttiey anttiey Apr 22, 2024

Choose a reason for hiding this comment

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

리뷰의 의도에 맞도록 변경한 것인지 모르겠지만 args 를 제거하니 조작할 수 있는 화면이 뜨지 않더라고요! 우선, 스토리북 테스트에서 사용되지 않는 args 를 제거하여 조작할 수 없도록 수정해 보았습니다.

812fcb9

@moonheekim0118
Copy link
Copy Markdown

moonheekim0118 commented Apr 18, 2024

UX 관련피드백

요거는 선택적으로 반영해주세요 :)

1. 현재 Input 에 text 도 입력되는데 의도하신걸까요?

아예 number 만 받도록 하면 어떨까 생각했어요ㅎㅎ
스크린샷 2024-04-18 오후 7 15 15

2. 카드 정보 입력시 실시간으로 카드가 업데이트 되면 좋을 것 같아요.

지금은 카드번호와 유효기간의 경우 input 유효성 검증이 끝난 후에 카드 프리뷰에 데이터가 보여지는데요, 입력하는 즉시 카드프리뷰에 해당 데이터가 반영되면 좋지 않을까요~?

@anttiey
Copy link
Copy Markdown
Member Author

anttiey commented Apr 22, 2024

안녕하세요, 호프! 피드백 반영이 늦었네요 🥹

피드백 반영 사항

폴더 구조

전반적인 폴더 구조를 변경하였습니다.

이번 기회에 현재 파슬리의 프로젝트에 적당한 폴더구조에 대해서 한번 고민해봐주시면 좋을 것 같아요.

피드백을 토대로 적절한 폴더 구조를 고민해 보게 되었습니다. 기존에는 호프의 언급대로 기능 별로 파일을 분리했는데요. 이렇게 되니 확실히 같은 컴포넌트를 관리하는 파일들이 떨어져 있어서 어려움이 생길 것 같다는 생각이 들었습니다.

- CardPreview(폴더)
  - CardPreview.tsx (컴포넌트)
  - CardPreview.stories.tsx (스토리북)
  - CardPreview.style.tsx (CSS)

따라서 위와 같은 형태로 폴더 구조를 변경해 보았어요. 추가로 공통 컴포넌트는 common 폴더 아래에 두어 공통 컴포넌트와 비즈니스 컴포넌트를 분리했습니다.

UX

현재 Input 에 text 도 입력되는데 의도하신걸까요?

유효성 검사를 거쳐서 숫자가 아닌 경우 유효성 메시지를 띄워 주기 때문에 따로 처리를 해주지 않았는데요. 기능 요구 사항에 입력 제한을 두어 사용자가 숫자만 입력할 수 있도록 한다 라는 내용이 있다는 걸 잊고 있었습니다 😅 현재는 사용자가 숫자가 아닌 입력을 하게 될 경우 입력되지 않도록 처리했습니다!

카드 정보 입력시 실시간으로 카드가 업데이트 되면 좋을 것 같아요.

기존에는 onChange 이벤트에서 길이 유효성을 함께 체크하여 숫자의 길이가 특정 조건(2 or 4)을 만족하기 전에는 프리뷰에 반영되지 않았습니다. 따라서 onBlur 이벤트를 활용하여 포커스가 해제될 때 유효성을 체크하도록 변경해 보았습니다.

나머지 피드백 반영 사항은 각 코멘트에 달아 두었습니다! 🙌🏻


추가 질문

setState 를 직접 주입하는 코드를 제거하고 상위 컴포넌트에 핸들러 함수를 만들면서 유효성 처리 위치에 대한 고민이 생겼습니다.

현재는 각 입력 컴포넌트(CardOwnerInput, CardNumberInput 등) 내부에 유효성 검사를 위한 변수와 함수를 모두 가지고 있습니다. 유효한 값만 핸들러 함수를 통해 상태를 업데이트하는 구조인데요. 이렇게 되니 핸들러 함수가 단순히 setState 함수를 감싸는 역할만 하고 별다른 로직이 없어 보입니다.

좀 더 구체적으로, 유효성 검사를 사용자 입력 컴포넌트에서 해야 할지 아니면 상태 업데이트 컴포넌트에서 해야 할지 고민입니다. 일반적으로 유효성 검사는 어디서 하는 게 좋다고 보시나요?

+) 남겨 주신 코멘트 중 '인풋 상태와 유효성 검사 상태 관리' 관련 코멘트가 있었는데, 해당 문제와 연관된 것 같아 함께 고민하여 반영해 보도록 하겠습니다...!

Copy link
Copy Markdown

@moonheekim0118 moonheekim0118 left a comment

Choose a reason for hiding this comment

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

안녕하세요 파슬리~! 반영해주신 사항 확인했어요. 적극적으로 리뷰 반영해주시고, 인사이트들도 공유해주셔서 정말 감사합니다 😊

폴더구조도 이전보다 훨씬 더 깔끔해진 것 같고, 역할에 맞게 잘 분리된 것 같아요 👍 👍 👍
추가로 질문 남겨주신 사항은 리뷰에 남겼으니 확인 부탁드릴게요 :)

리뷰 남겨드린 사항들은 2단계에서 같이 진행해도 괜찮을 것 같아서 1단계는 여기서 마무리하려고 합니다.
개선하시느라 고생하셨어요 :) 2단계에서 뵈어요~ 🙂

Comment thread src/App.tsx
<h1>React Payments</h1>
</>
<ThemeProvider theme={theme}>
<GlobalStyles></GlobalStyles>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

self closing tag 사용 가능해보입니다ㅎㅎ

Suggested change
<GlobalStyles></GlobalStyles>
<GlobalStyles/>

관련 eslint 룰 추가해도 괜찮을 것 같아요 :)

Comment on lines +21 to +33
useEffect(() => {
if (!isValidMonth) {
setErrorMessage(ERROR_MESSAGE.INVALID_EXPIRATION_MONTH);
return;
}

if (!isValidYear) {
setErrorMessage(ERROR_MESSAGE.INVALID_EXPIRATION_YEAR);
return;
}

setErrorMessage('');
}, [isValidMonth, isValidYear]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

요거 effect 로 관리된 이유가 있을까요?
effect 는 말그대로 부수효과 (side-effect)여서, 컴포넌트 내에서 최소화 하는게 좋아요. (물론 잘 사용하면 최고지만용)
디펜던시 관리도 해줘야하고..useEffect 실행시점도 디버깅하기가 까다롭기도 하고요.

현재는 그냥 validation 처리하는 곳에서 errorMessage까지 set 해줘도 괜찮을 것 같긴 하네요ㅎㅎ

Comment on lines +17 to +20
const [isValidMonth, setIsValidMonth] = useState(true);
const [isValidYear, setIsValidYear] = useState(true);
const [errorMessage, setErrorMessage] = useState('');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

좀 더 구체적으로, 유효성 검사를 사용자 입력 컴포넌트에서 해야 할지 아니면 상태 업데이트 컴포넌트에서 해야 할지 고민입니다. 일반적으로 유효성 검사는 어디서 하는 게 좋다고 보시나요?

파슬리가 남겨주신 질문 여기에서 이어가볼게요ㅎㅎ

저번에 드린 리뷰에 추가 설명을 하자면 요거는 추후에 submit 로직이 들어온다고 생각해보시면 답이 간단한데요,
submit 할 때 form에서 각 인풋 필드의 value 와 값의 유효 여부를 알고 있어야하겠죠? 그러면 결국 필드당 value & isValid 상태는 같은 레벨에서 함께 관리되어야 할 정보일 것 같아요.

그런데 현재 구조상 isValid 상태는 value 상태보다 하위레벨에서 관리되고 있으니, 추후에 form 이 위치한 상위 컴포넌트에서는 이에 접근 할 방법이 없을 것 같구요!

이렇게 상태의 위치가 위로 올라간다면 자연스럽게 유효성 검증 로직도 상위에서 관리되는게 자연스러울 것 같고요ㅎㅎ

@moonheekim0118 moonheekim0118 merged commit df70c6c into woowacourse:anttiey Apr 23, 2024
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