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

[넥스트레벨 Outwater] 페이먼츠 미션 STEP1 #10

Open
wants to merge 9 commits into
base: Outwater
Choose a base branch
from

Conversation

Outwater
Copy link

@Outwater Outwater commented Jan 27, 2022

Review

데모 환경 바로가기

VSCode환경 바로가기

안녕하세요, 서인수입니다.
storybook을 제외하고 일반 기능구현까지는 완료하였습니다.
코드 보시면서 조금이라도 읽기 어렵거나, 이해 어려운 부분 있으면 코멘트 남겨주시면 정말 좋을 것 같아요.(가독성 측면)
남이 읽기 편한 코드를 작성하고 싶은데, 제 코드는 스스로 많이 보다보니 적응이 되어서 찾기 어려운 것 같아요 ㅠㅠ

이번 Step을 진행하면서 어려웠던 점 3가지 정도 뽑아봤는데, 의견말씀해주시면 큰 도움 될 것 같아요 ㅎㅎ : )

  • 코드 작성하기 전 어느정도 구조 설계를 하시는 지 궁금합니다.

    • 저의 경우, page별로 먼저 나누어 생각하고, 컴포넌트 단위를 먼저 생각해본 뒤(Card, CardForm), 컴포넌트에서 필요한 상태는 무엇이 있을까 정도를 생각하고 작성했습니다.
    • formData의 type에 대한 설계미스로 난항을 겪었습니다.
  • 전체적으로 코드가 정해진 기능구현에 초점이 맞추어져있어 유연하지 못하다는 생각이 제가 작성을 하는 내내 들었습니다. cardData의 상태를 객체형식으로 지정하고, 각 객체의 중첩정도가 다르다보니, 이 cardData를 동적(?)으로 모듈화하여 사용하기 어렵지 않았나 생각합니다..

  • 모든 input 마다 각각의 onChange핸들러가 존재하고, 또 각각의 onChange핸들러는 validation 체크, 상태변경(setState), focus이동등 많은 역할을 담당하고 있다.(우겨 넣은 느낌..)

  • focus이동하는 코드나 error를 띄우는 코드를 모듈화할 수 있지 않을까? 생각이 드는데, 아직은 어떻게 해야할 지 감이 오지않아 부족함을 느꼈습니다..

  • (리뷰) validation관련 로직이 퍼져있는데, 1) 길이는 input 태그가 가지고 있는 속성으로, 2) onChange시 dataType에 대한 검증 (number인지) 3) 다음 페이지 이동 시, 빈 값이 있는지 검증, 효율적인 validation 방안이 있을지 궁금합니다..

📝 Requirements

카드 추가

  • <(뒤로가기) 버튼 클릭 시, 카드 목록 페이지로 이동한다.
  • 카드 번호를 입력 받을 수 있다.
    • 카드 번호는 숫자만 입력가능하다.
    • 카드 번호 4자리마다 -가 삽입된다.
    • 카드 번호는 실시간으로 카드 UI에 반영된다.
    • 카드 번호는 앞 8자리만 숫자로 보여지고, 나머지 숫자는 *로 보여진다.
    • 카드 번호 앞 8자리로 카드사를 추정하여 그 테마를 카드 UI에 반영한다.
    • 카드사가 선택되고 유효한 카드 번호 16자리를 모두 입력하면, 자동으로 만료일로 focus된다.
    • 카드 앞 8자리 숫자를 입력하고 카드사가 선택되지 않은 경우, 나머지 카드 번호 입력 시도 시, 카드사 선택 모달이 보여진다.
    • 유효하지 않은 카드 번호를 입력하면, 입력 폼 아래에 에러 메시지를 보여준다.
  • 만료일을 입력 받을 수 있다.
    • MM / YY 로 placeholder를 적용한다.
    • 월, 년 사이에 자동으로 /가 삽입된다.
    • 만료일은 실시간으로 카드 UI에 반영된다.
    • 월은 1이상 12이하 숫자여야 한다.
    • 월, 년 입력이 유효하면 보안코드 입력으로 focus된다
    • 유효하지 않은 만료일을 입력하면, 입력 폼 아래에 에러 메시지를 보여준다.
  • 보안코드를 입력 받을 수 있다.
    • 보안코드는 *으로 보여진다.
    • 보안코드 3자리가 입력되면 카드 비밀번호 입력으로 focus된다.
    • 보안코드는 숫자만 입력가능하다.
    • 유효하지 않은 보안코드를 입력하면, 입력 폼 아래에 에러 메시지를 보여준다.
  • 카드 비밀번호의 앞 2자리를 입력 받을 수 있다.
    • 카드 비밀번호는 각 폼마다 한자리 숫자만 입력가능하다.
    • 첫자리 입력이 완료되면 둘째자리 입력으로 focus된다.
    • 카드 번호 입력 시, *으로 보여진다.
    • 유효하지 않은 카드 비밀번호를 입력하면, 입력 폼 아래에 에러 메시지를 보여준다.
  • 카드 소유자 이름을 입력 받을 수 있다.
    • 이름은 30자리까지 입력할 수 있다.
    • 이름 입력 폼 위에, 현재 입력 자릿수와 최대 입력 자릿수를 실시간으로 보여준다.
  • 클릭 시, 카드 등록 완료 페이지로 이동한다.

심화 요구사항 (선택사항)

  • Storybook 단위 테스트
  • 유효성 검증 실패에 대한 UI/UX 추가
  • 카드사 선택
    • 카드사를 선택하면 모달이 닫히고, 그 테마를 카드 UI에 반영한다.
    • 카드사를 선택하지 않아도 모달을 닫을 수 있다.
  • 보안코드 툴팁
    • 클릭 시, 보안코드 관련 안내 메시지를 보여준다.
    • focusout 시, 툴팁이 닫힌다.
  • 가상 키보드
    • 마스킹 처리된 값 입력시 사용
    • 숫자를 랜덤으로 배열

@Outwater Outwater changed the base branch from main to Outwater January 27, 2022 11:43
@Outwater Outwater changed the title Outwater [넥스트레벨 Outwater] 페이먼츠 미션 STEP1-1 Jan 27, 2022
@Outwater Outwater changed the title [넥스트레벨 Outwater] 페이먼츠 미션 STEP1-1 [넥스트레벨 Outwater] 페이먼츠 미션 STEP1 Jan 27, 2022
@Outwater Outwater requested a review from woobottle January 27, 2022 11:49
@Outwater Outwater self-assigned this Jan 27, 2022
@Outwater
Copy link
Author

@woobottle gh-pages 배포하여 데모링크 추가하였으니, 리뷰하실 때 데모환경에서 동작확인 해보셔도 좋을 것 같습니다.

@woobottle
Copy link

저의 경우, page별로 먼저 나누어 생각하고, 컴포넌트 단위를 먼저 생각해본 뒤(Card, CardForm), 컴포넌트에서 필요한 상태는 무엇이 있을까 정도를 생각하고 작성했습니다.
=> 최근 부터는 저 또한 page별로 프로젝트 구조를 설계하려고 합니다.
Page && 경로 별로 파일을 생성하여 구조를 파악하기 쉽게 하고 같은 성격의 파일들은 한곳에 묶어 두려 합니다.
그중 공통되는 컴포넌트 들만 shared 폴더를 생성해서 이 안에 위치시키려 합니다.

(리뷰) validation관련 로직이 퍼져있는데,

  1. 길이는 input 태그가 가지고 있는 속성으로,
  2. onChange시 dataType에 대한 검증 (number인지)
  3. 다음 페이지 이동 시, 빈 값이 있는지 검증, 효율적인 validation 방안이 있을지 궁금합니다..

=> 1번 2번 같은경우
input 태그의 pattern과 data속성들을 결합해서 사용하는 것은 어떨까요?? regex에 대한 것은 각 input에서 요구하는 것이 다르기 때문에 각각 정의후에 input태그에 알맞게 넣어주는 방법밖에 없을것 같아요. 이걸 pattern에 명시해주는거죠

3번 form.checkValidity() method를 활용할수 있는 방안은 없을지 궁금합니다.

src/App.jsx Show resolved Hide resolved
src/components/Card.jsx Show resolved Hide resolved
src/utils/index.js Show resolved Hide resolved
src/components/Card.jsx Show resolved Hide resolved
src/components/Card.jsx Show resolved Hide resolved
navigate("/alias");
};

const [modalView, setModalView] = useState(false);

Choose a reason for hiding this comment

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

위 아래 로직과 동떨어진 상태에서 선언된게 아닌가 싶어요.
자칫하면 파악하기 어렵지 않을까 싶어서요

선언 위치가 좀 더 파악하기 쉬운곳이면 어떨까 싶습니다. (ex. 최상단 or 최하단)

src/components/CardForm.jsx Show resolved Hide resolved
src/components/CardForm.jsx Show resolved Hide resolved
src/components/CardForm.jsx Show resolved Hide resolved
import Input from "../components/Input";
import Modal from "../components/Modal";
import { findCompany } from "../utils/index.js";

Choose a reason for hiding this comment

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

가독성 부분에 대해 언급해주신 부분은 아래의 부분때매 언급해주신거라 판단이 되어요

처음 접했을때는 코드 양이 너무 많아서 두려운 마음이 컸지만
읽다 보니 흐름의 이해가 어렵지 않았습니다.

제가 추측하기에 가독성 언급이 나온것은 Input의 onChange로 인해서 코드양이 방대해진것 같아요.
추상화 레벨을 한단계 높여서 type에 따른 Input의 구분과 onChange를 form에 거는것은 어떨까요?
가독성을 해결할수 있는 방법중 하나가 코드의 추상화 레벨을 높여서 관리하는 것이 아닐까 생각해요.
대신 파일 전체의 추상화 레벨이 같게 유지되어야 해서 어려울수 있는 작업이라 생각합니다.
그렇다면 후자의 form의 onchange로 가는게 나쁘지 않을까 해요

formik의 경우 input들의 handler를 form에 걸어서 활용한 것으로 알고있습니다(혹시 아니라면 말씀해주세요!!)

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다. 우병님 코드에서 추상화수준을 맞춰서 가독성이 훌륭한 것을 보고 많이 감명을 받아서, 배워보고 싶어 이번 리팩토링에 유사하게 적용해보았습니다.
추상화수준이 유사하고, 외부에서 주요 로직을 확인할 수 있다면 가독성이 많이 증가되는 것 같아요.
많이 배웠습니다. 감사합니다 👍

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