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

[2단계 - 페이먼츠] - 초코(강다빈) 미션 제출합니다. #405

Merged
merged 66 commits into from
Apr 30, 2024

Conversation

00kang
Copy link
Member

@00kang 00kang commented Apr 29, 2024

안녕하세요 피터! 초코🍫입니다.
페이먼츠 2단계 미션 리뷰 잘 부탁드립니다. !!

🔗 배포 링크

페이먼츠 페이지

🔑 키워드 및 학습 목표

  • hooks, Form Control, State
  • 다양한 Form 구성 요소들간의 상태를 효율적으로 관리한다.
  • hooks API를 이용하여 상태 관리 로직을 구현하한다.
  • custom hooks를 생성하여 Form 관리 로직을 컴포넌트에서 분리하고 재사용한다.
  • Controlled & Uncontrolled Components에 입각하여 Form을 핸들링한다.

😮‍💨 상황공유

저는 우아한테크코스를 시작하며 웹 프론트엔드에 입문했에요. 그래서 자바스크립트에 대한 이해도도 낮고, 리액트도 아주 얕게 학습해보았고, 기본 훅인 useState와 useEffect만 사용해본 상태에요. 그래서 스텝2 시작시점이 다른 크루보다 늦었음에도 리액트의 기본을 공부한 뒤 기능 구현을 해야겠다는 생각(아주 잘못된 생각인 것 같죠,,?) 에 기능 요구 사항을 다 충족시키지 못한 쓰레기 코드가 되어버렸어요 ㅠㅠ 이 점 참고하여 리뷰해주시면 감사하겠습니다!

❓ 질문사항

  1. 리액트를 처음 공부할 때 어떻게 공부하셨나요? 앞서 말씀드렸다싶이 리액트를 공부하고 기능을 구현하는 것은 안 그래도 기능 구현 속도가 느린 저에게 맞지 않는 방법이었다고 생각합니다. 피터라면 처음 리액트에 입문했을 때 미션을 수행하며 공부하려면 어떻게 해야 좋다고 생각하시나요? 유용했던 학습 방법이 있다면 공유해주세요.

  2. 현재 아주 작은 기능 단위로 쪼개어 차근차근 했어야 했는데 저에게는 미션이 어려웠다보니 코드가 정말 꼬여버렸는데요,, 이렇게 꼬여버리니 어디서부터 손을 대야할지 막막해서 시간을 흘려보냈습니다ㅜ 코드의 오류가 난거라면 오류 메시지를 검색하여 해결하는 방법을 찾으면 되겠지만, 오류는 안나지만 제대로 된 동작을 하지 않을 때는 어떻게 풀어나가야 할까요??

  • 현재 코드에서 카드번호 입력이 이뤄지지 않는 문제
  • 현재 코드에서 카드 소유자이름 입력폼부터 나머지 폼이 한번에 나타나는 문제
    이 두가지 문제부터 제대로 동작하게끔 하고 싶은데, 어떤 부분을 먼저 살펴보면 될까요?
  1. 현재 코드에서는 동적 ui를 위해 각 폼마다 유효한지 파악하는 state를 추가했습니다. 상태관리 요소가 많아서 복잡하다고 느끼는데 지금 상황에서 어떤 것부터 시도해보면 좋을까요? 이미 저에게 복잡하다고 느껴지는 상황에서 무엇부터 시작해보면 좋을지 막연해서 여쭤봅니다...!

00kang and others added 30 commits April 24, 2024 19:20
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
- credit card 번호 확인 유효성 검사
- credit card 유효기간 유효성 검사
- credit card 소유자 이름 유효성 검사

Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
Co-Authored-By: Lee sang Yeop <mma7710@naver.com>
- 드롭다운 오픈후 카드사 선택하지 않으면 에러 발생
- 선택된 카드사에 따라 카드 배경색 전환
- 숫자인지 체크하는 checkIsDigit 메서드
@iborymagic iborymagic self-assigned this Apr 29, 2024
@iborymagic
Copy link

iborymagic commented Apr 30, 2024

아이고 쓰레기코드라니요; 누구에게나 처음은 있는거니까 자책 안하셔도 됩니다.
적어도 제가 리액트 처음 사용했을 때보다는 5만배는 잘하고 계십니다.

  1. 저도 비슷한 경험을 한 적이 있었는데, 저는 애초부터 배운다는 마음가짐으로 미션보다 리액트 공식문서를 학습하는 데에 우선순위를 두고 진행했었습니다. 코치님들께는 죄송하지만(ㅎㅎ..), 사실 리액트를 알지 못하면 애초에 미션 진행 자체가 어려운 상황이잖아요? 그래서 지금도 input에 입력이 안되는 채로 미션을 제출할 수밖에 없었을거구요. 그래서 저는 얼굴에 철판을 깔고.. 리액트 공식문서를 동료와 함께 정독하는 것부터 시작했습니다. 물론 뒷감당은 초코가 하셔야해요 😋

  2. 말씀 주신대로, 아무런 에러가 발생하지 않는데 원하는 대로 동작하지 않는 경우를 디버깅하는 게 참 어렵습니다. 저는 코드 단계마다 전부 막무가내로 console.log를 찍어가며 제가 의도한 대로 값들이 올바르게 전달되고 있는지를 하나하나 해부해봤던 것 같습니다. 로직 단위로 주석을 쳐가며 어떤 로직이 문제인지 관찰해보기도 했구요. 사실 지금도 가끔 감이 안잡힐 때는 그렇게 하기도 합니다. 때로는 가장 무식한 방법이 가장 효과적이더라구요.

  3. form을 다루다보면 상태가 많아지고 로직이 복잡해지면서 어쩔 수 없이 코드가 북적북적해지기 마련입니다. 상태나 로직이 너무 복잡하다면, hook을 활용하여 컴포넌트단의 로직을 간단하게 만들어보는 연습을 하면 좋을 것 같아요. 감이 잘 잡히지 않는다면 다른 크루들은 어떻게 했는지 살펴보면 도움이 될 것 같습니다.

Copy link

@iborymagic iborymagic left a comment

Choose a reason for hiding this comment

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

안녕하세요 초코! 2단계 미션 진행하시느라 고생 많았습니다.
디버깅하기 어려운 문제이긴 하지만, 관련해서 리뷰를 남겨놨으니 확인해보시고 스스로 한 번 디버깅을 해보시면 많은 도움이 될거라고 생각합니다. useEffect를 줄이는 것에 대해서는 꼭 이번이 아니더라도 차츰차츰 꼭 시도해보시면 코드를 개선하는 데에 큰 도움이 될거라고 생각해요.

벌써 다음 미션이 시작된 상태이기 때문에 일단은 여기서 머지를 하도록 하겠습니다.
만약 추가로 리뷰 받고싶은 내용이 있다면 코멘트를 달고 DM을 보내주시면 바로 확인해보도록 하겠습니다. 고생 많았어요~
(질문주신 내용은 별도 코멘트로 달아놓았으니 확인 부탁드려요)

import { CardNumberValue } from "../../../@types/CreditCard";
import CARD_FORM_MESSAGE from "../../../constants/cardFormMessage";
import CARD_INPUTBOX_NAME from "../../../constants/cardInputBoxName";
import InputBox from "../common/InputBox";

interface InputCreditCardNumberProps {

Choose a reason for hiding this comment

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

사실 요 컴포넌트는 굳이 한 번 더 분리할 필요는 없을 것 같다는 생각입니다. CardNumberForm 에 그대로 집어넣어도 코드가 크게 복잡하지 않아보이는데, 굳이 depth만 하나 더 생긴 느낌이에요. (나머지 Input 컴포넌트들도 마찬가지)

Comment on lines +92 to +94
useEffect(() => {
handleCardNumberValidity();
}, [cardNumber, cardNumberError]);

Choose a reason for hiding this comment

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

요런 로직은 useEffect가 아닌, onChange handler 내부에서도 충분히 수행할 수 있을 것 같아요!

Comment on lines +19 to +23
console.log(inputValue, inputValue.name);
console.log("-----", isOwnerValid);
setOwner(inputValue);
setIsOwnerValid(isOwnerValid && !ownerError);
console.log("!!!!!", { inputValue, ownerError, setOwner, setIsOwnerValid });

Choose a reason for hiding this comment

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

이미 수많은 console.log 디버깅의 흔적들이.. 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

콘솔로그를 지웠어야 했는데.. ㅠ

Comment on lines +26 to +29
useEffect(() => {
setCVC(inputValue);
setIsCVCValid(!cvcError);
}, [inputValue, cvcError, setCVC, setIsCVCValid]);

Choose a reason for hiding this comment

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

전체적으로 이벤트 핸들러로 대체 가능한 useEffect들이 불필요하게 사용되는 감이 없지않아 있어서, 요런 것들은 차츰 개선해나가보면 좋을 것 같네요.

Choose a reason for hiding this comment

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

사실, 현재 input이 입력되지 않는 게 디버깅이 힘든 이유도 useEffect가 적지 않은 비중을 차지하는 것 같습니다. 이렇게 불필요한 useEffect를 그것도 여러 군데에서 사용하다보면, 데이터의 흐름이 파악되지 않아 어디서부터 디버깅을 해야할지 감이 잡히지 않기 마련입니다. 일단 버그부터 고치고나서, useEffect를 걷어내는 연습을 해보면 좋을 것 같아요~

https://react.dev/learn/you-might-not-need-an-effect 요거는 꼭 읽어보시구요!

Comment on lines +20 to +26
interface CVC {
number: string;
}

interface Password {
number: string;
}

Choose a reason for hiding this comment

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

타입이 중복으로 존재하는 것 같아요!

Comment on lines 31 to +41
checkCreditCardNumber(value: string) {
const maxDigit = 4;

if (ValidatorCondition.checkMaxDigit(value, maxDigit))
return { isError: false, isValid: false };

if (!ValidatorCondition.checkIsDigit(value)) {
return { isError: true, isValid: false };
}
if (ValidatorCondition.checkMaxDigit(value, maxDigit))
return { isError: false, isValid: false };

return { isError: false, isValid: true };
return { isError: false, isValid: value.length === maxDigit };
},

Choose a reason for hiding this comment

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

어떤 로직이 문제인지를 보기 위해 한 덩어리씩 주석 처리를 해가며 확인해봤는데, validation 로직에 문제가 있어서 값이 반영되지 않는 것 같네요. useInput에서 validation 이후의 로직이 실행되지 않는걸로 봐서는 isError 혹은 isValid 값에 뭔가 문제가 있는 것 같습니다.

Choose a reason for hiding this comment

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

근데 혹시 isError와 isValid를 따로 가져가는 이유가 무엇인가요?

Comment on lines +14 to +23
const options = [
{ value: "bcCard", label: "BC카드" },
{ value: "shinhanCard", label: "신한카드" },
{ value: "kakaoCard", label: "카카오뱅크" },
{ value: "hyundaiCard", label: "현대카드" },
{ value: "wooriCard", label: "우리카드" },
{ value: "lotteCard", label: "롯데카드" },
{ value: "hanaCard", label: "하나카드" },
{ value: "kbCard", label: "국민카드" },
];

Choose a reason for hiding this comment

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

요거는 상수로 빼면 좋을 것 같아요~

@iborymagic iborymagic merged commit 91adf9a into woowacourse:00kang Apr 30, 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