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

[1단계 - 페이먼츠 미션] 하루(김하루) 미션 제출합니다. #15

Merged
merged 94 commits into from May 4, 2021

Conversation

365kim
Copy link

@365kim 365kim commented Apr 28, 2021

자프님, 안녕하세요!

오랜만에 인사드리는 🙋🏻‍♀️ 하루(@365kim) 입니다.
지난 레벨1 때도 유튜브강의실 리뷰를 도와주셨었는데 요번에는 페이먼츠 미션 으로 인사드리게 되었네요~!
저는 신세한탄(@shinsehantan)과 함께 페어프로그래밍을 진행했습니다.


💪 새롭게 시도한 부분

이번 미션은 스토리북컴포넌트 주도 개발 을 처음 접하고 익혀보는 시간이었습니다.
💫 재사용가능한 컴포넌트 💫 를 만들고자 의도했지만 결과물이 잘 나왔는지는 잘 모르겠습니다... 🥲
크고 작은 개선사항 코멘트로 전해주시면 열심히 개선해보겠습니다!

요번 미션도 잘부탁드리겠습니다! 🙇🏻‍♀️🙇🏻‍♀️🙇🏻‍♀️


💳 1단계 데모 페이지

카드 목록 관리는 2단계 구현사항이라, 1단계에서는 우선 단 하나의 카드 상태만을 관리할 수 있도록 구현했습니다...!


📁 디렉토리 구조

Group 103


✅ 구현 기능 목록

카드 목록 화면

  • 카드추가 버튼(이미지)을 클릭하면 카드 추가 폼 화면이 표시된다.

카드 추가 폼 화면

  • 카드번호
    • 카드 번호 입력창이 자동 포커싱된다.
    • 네 자리 입력이 완료될 때마다 하이픈(-)이 표시되고 다음 입력창으로 넘어간다.
    • 8자리까지 입력했을 때 8자리로 매칭되는 카드사가 있을 경우, 카드사가 자동으로 선택된다.
    • 8자리까지 입력했을 때 8자리로 매칭되는 카드사가 없을 경우, 카드사 선택 모달이 표시된다.
      • 카드사 버튼을 클릭해서 카드사를 선택할 수 있다.
      • 모달의 Dimmed 영역을 클릭해서 모달을 닫을 수 있다.
    • 9~12번째 자리와 13~16번째 자리 입력 시 마스킹되어 표시된다.
    • 16자리를 모두 입력하면 카드 이미지에도 카드번호가 표시된다.
    • 16자리를 모두 입력하면 카드 만료일 입력창으로 포커스가 넘어간다.
  • 카드 만료일
    • 월 2자리 입력이 완료되면 연 2자리 입력창으로 포커스가 넘어간다.
    • 4자리를 모두 입력하면 카드 소유자 이름 입력창으로 포커스가 넘어간다.
    • 4자리를 모두 입력하면 카드 이미지에도 카드번호가 표시된다.
  • 카드 소유자 이름
    • 입력된 글자수가 우측 상단에 실시간으로 표시된다.
    • 알파벳만 입력 가능하다.
    • 소문자로 입력하더라도 input창에는 대문자로 표시된다.
    • 이름을 입력하고 포커스아웃 되었을 때 카드 이미지에도 이름이 표시된다.
  • 보안 코드
    • 입력 시 마스킹되어 표시된다.
    • 물음표 버튼 호버 시 보안 코드 설명 모달이 표시된다.
    • 3자리를 모두 입력하면 카드 비밀번호 입력창으로 포커스가 넘어간다.
  • 카드 비밀번호
    • 입력 시 마스킹되어 표시된다.
  • 카드 등록
    • 선택 칸을 제외한 빈 칸이 없고, 카드사도 선택이 된 경우에 '다음'버튼이 표시된다.
    • '다음'버튼을 클릭하면 카드를 등록한다.
      • 카드사가 선택되지 않았을 경우, 카드사 선택 모달을 표시한다.

카드 추가 확인 화면

  • 등록 카드 확인
    • 등록한 카드의 이미지가 표시된다.
  • 별칭 등록
    • 별칭 등록에 자동 포커싱된다.
    • 1글자 ~ 10글자 범위 내로 입력 가능하다.

365kim and others added 30 commits April 20, 2021 17:39
Co-authored-by: 365kim <365kim@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Co-authored-by: shinsehantan <shinsehantan@users.noreply.github.com>
Copy link

@wow9144 wow9144 left a comment

Choose a reason for hiding this comment

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

컴포넌트를 재사용 하는데 초점을 많이 맞춰서 진행하신 것 같습니다.
코멘트 내용이 많은데 참고하시어 재사용 가능한 좋은 컴포넌트를 설계하는데 도움이 되었으면 좋겠습니다.

src/App.js Outdated
return (
<div className="App">
{route === PAGE.CARD_LIST ? (
<CardListPage cardList={cardList} setRoute={setRoute} />
Copy link

Choose a reason for hiding this comment

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

react-router 를 굳이 쓰지 말라는 제약이 있던게 아니라면 도입하시면 좋을 것 같습니다.

react-router 를 쓰지 않는다고 하더라도 현재처럼 3항식으로 라우팅 결정을 하면,
CardListPage 와 AddCardPage 말고 또 다른 Page가 추가 되었을 때, 그 Page만 추가하면 쉬울텐데 새로운 Page가 추가되었는데 기존에 만들어둔 라우팅 로직을 수정해야 하는 상황이 벌어지게 됩니다.

그리고 잘 한 번 생각해보시면 PAGE.CARD_LIST 가 아니면 CardListPage 가 아닌 다른 Page이지,
PAGE.CARD_LIST가 아니면 AddCardPage 이다 라고 하는 논리도 사실 이상합니다. :)

Copy link
Author

@365kim 365kim May 3, 2021

Choose a reason for hiding this comment

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

WATCHING_TYPE, WATCHED_TYPE처럼 열거형으로 작성하더라도 삼항연산자를 써버리면 결국 true/false로 한 것과 다르지 않다. 열거형으로 작성하되 삼항연산자를 함께 사용하지 않는 것이 확장성에 유리하다.

유튜브 강의실 미션 당시, 자프님께서 다른 크루의 PR(#36)에서 이미 지적해주셨던 부분과 비슷한 실수를 했네요..! 말씀해주신대로 react-router-dom을 적용해 보았습니다. (참고: reactrouter.com) 현재 홈화면이 카드목록 화면인데, 홈화면이 변경될 경우를 대비해서 ROUTE.HOME과 ROUTE.LIST를 따로 두어 관리하도록 하였습니다.

현재 카드 별칭까지 추가하는 부분까지 라우팅이 정상적으로 이루어지는데 다시 홈화면으로 돌아가려는 경우에 리스트가 정상적으로 렌더되지 않는 문제가 있습니다. (state는 잘 저장되고 있는데 컴포넌트 렌더는 안되네용...🥲) react-router-dom의 API를 좀 더 숙지해서 카드 목록을 완성하는 2단계에서 수정해보도록 하겠습니다...!! 💪💪 수정했습니다! ㅎㅎ

📌관련커밋 ab9bce7, 4630b1f

@@ -0,0 +1,12 @@
import { PAGE, MAX_NICKNAME_LENGTH } from '../../../../constants';
Copy link

Choose a reason for hiding this comment

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

handler.js 로 별도 파일로 빼서 얻는 이득보다는 실이 더 클것 같습니다.
실이 더 클 것 같은 이유로

  1. 이 핸들러들이 재사용 가능할 것이라고 생각되지 않습니다.
  2. 파라미터는 컴포넌트가 알고 있으므로 컴포넌트 내에서 직접 의존해도 되는데, 외부로 빼면 이 파라미터들을 위해 일일이 전달 해줘야 합니다.(재사용 가능한 함수라면 전달해서라도 쓰면 되겠겠지만 함수내에 존재하는 로직과 전혀 추상화 되지 않은 파라미터의 형태를 볼 때 재사용 가능한 형태로 보기 어렵습니다. )

아래 CardNicknameForm 에 추가 코멘트 있습니다.

Copy link
Author

Choose a reason for hiding this comment

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

handler.js 파일 분리는 가장 처음 작성한 CardNumberInput.js 가 100줄이 훌쩍 넘어가서 ‘으아아 너무 길다... 분리해서 보자!’ 라고한 것이 시초였습니다… 그 후 'CardNumberInput 에서도 분리했는데 다른 것들도 다 맞춰서 분리해야하지 않을까?' 라는 단순한 이유로 짧은 길이의 코드도 모두 별도의 핸들러 파일을 들고 있게 되었습니다…🥲

리뷰어님의 피드백을 보고나서야, 모듈의 재사용성을 고려하지 않고 단순 분리한 것이었음을 깨닫고 핸들러 함수들을 다시 index.js 파일 안으로 이동시켰습니다….!

그런데 핸들러 함수를 컴포넌트 안으로는 넣지 않고 분리해서 보는게 가독성이 더 좋지 않을까 하는 생각이 들었습니다...! 핸들러 함수는 적게는 5줄 길게는 10줄이 넘고, 다수의 핸들러를 갖는 컴포넌트도 있었습니다. 이런 핸들러 함수를 선언부에 넣어서 컴포넌트의 index.js 파일에 진입했을 때 핸들러 함수의 구체적인 정의를 보기보다는, 컴포넌트의 가장 중요한(?) 부분인 return값을 볼 수 있게 하는게 어떨까 싶었습니다.

또 핸들러 함수를 컴포넌트 내부에서 정의하면 해당 컴포넌트가 호출될 때마다 함수가 다시 만들어지는 것 같았습니다. 웹팩이 모듈을 합치고나서 정확히 어떻게 평가하고 실행하는지는 잘 모르겠어서 모듈스코프에 콘솔로그를, 컴포넌트 내부에 콘솔로그를 찍어보았습니다. 그랬더니 모듈스코프의 콘솔은 1번찍히고, 컴포넌트 내부의 콘솔은 컴포넌트가 호출될 때마다 찍혔습니다. (당연한걸까요…? 제가 초보라서… 찍어보고 알았습니다…🥲 ) 그래서 컴포넌트 밖에 함수를 정의해두면 함수를 매번 만들지 않고 재사용할 수 있는 것 같습니다..!

말씀해주신 것처럼 다수의 handler.js 파일을 만들면서 컴포넌트에서 받은 props를 다시 일일이 다시 전달해주어야 하는 번거로움이 있었지만(매우매우 있었지만...), 위와 같은 이유로 handler.js 파일은 없애되, 컴포넌트 내부가 아닌 컴포넌트 하단에 함수선언문으로 작성해두었습니다…! 혹시 제가 잘못생각한 부분이나 놓친 부분이 있다면 살포시 말씀해주시면 감사하겠습니다…!! 🙇🏻‍♀️🙇🏻‍♀️🙏

📌 관련커밋: b5c2d97

Copy link

Choose a reason for hiding this comment

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

잘못 생각한 부분 없이 모두 맞습니다. :)

  1. 다만 핸들러 함수가 길다면 혹시 하나의 함수에서 여러가지 일을 하고 있어서 그런것은 아닌가 의심 해볼 필요가 있고, 함수가 많다면 이 컴포넌트의 책임이 너무 많아서 그런건 아닐까 컴포넌트를 더 잘게 쪼갤 수 있지 않을까 차원에서 한 번 더 생각해볼 필요는 있겠습니다.
  2. 컴포넌트 안에 선언된 함수가 있으면 렌더가 될 때마다 매번 새로 생성되는게 맞습니다. 그래서 성능에 영향을 주고 이런게 다 비용 아닐까 하는 생각이 드는것까지도 좋습니다. 👍 근데 그 비용이 크지 않습니다. 크지 않은 정도가 아니라 작고 작고 매우*100000 작아서 아무 신경 쓰지 않아도 될 정도로 작습니다. 이런게 걱정이 되어서 이런걸 해결 하는 차원에서의 결정을 한다면 최소한 이 걱정은 머릿속에서 지우고 결정을 해야 합니다.

컴포넌트 하단에 선언하신걸로 일단 가보시죠 :)

Copy link
Author

Choose a reason for hiding this comment

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

신경쓰지 않아도 될 정도로 작은 비용이군요! 설명해주신 덕분에 새로 배워갑니다. 자세한 설명 감사드립니다! 💙

/>
<Button
disabled={nickname === initialNickname}
onClick={(e) => handleNicknameSubmit({ e, setRoute, cardInfo, addCardInfoToList })}
Copy link

Choose a reason for hiding this comment

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

CardNicknameForm 컴포넌트 내에

const handleNicknameSubmit = ( e ) => {
  e.preventDefault();
  addCardInfoToList(cardInfo);
  setRoute(PAGE.CARD_LIST);
};

이렇게 선언하고

<Button onClick={handleNicknameSubmit}>
  확인
</Button>

이렇게 사용하는 것이 더 간결하고 관리 하기에도 좋아 보입니다.

border-radius: 50%;
}

.Button--card-company {
Copy link

Choose a reason for hiding this comment

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

Button--card-company 이런 클래스명의 스타일은 Button의 스타일이 아니므로 이곳에 존재하면 안됩니다.
Button이 어디서나 사용 가능한 컴포넌트가 맞다면 그러하고, 어디서나 사용가능한 버튼이 아니라면 Button이 아니라 그 상황에 맞는 좀 더 구체적인 이름이 되어야 합니다.
이 Button/style.css 에 존재하는 클래스명(스타일)들은 부트스트랩이나 기타 CSS 프레임워크처럼 이 프로젝트와 무관하게 완전히 분리되어 나가도 사용 할 수 있는 범용적인 형태가 되어야 합니다.
사실 여기서
.Button {
margin: 0.25rem;
border: none;
cursor: pointer;
}
말고는 다 지워지는게(옮겨지는게) 옳습니다.

Copy link
Author

@365kim 365kim May 3, 2021

Choose a reason for hiding this comment

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

./components 하위에는 오직 범용적이라고 할 수 있는 스타일만 남겨두고, 도메인과 관련된 스타일이 ./components 하위에 내에 남아있지 않도록 신경써서 이사시켰습니다! 재사용 목적으로 만든 컴포넌트의 스타일시트 관리 노하우를 더 알아갈 수 있는 시간이었습니다. 좋은 피드백 감사드립니다! 🙏

📌관련커밋: 398f431

height: 2.8125rem;
}

.CardPasswordInput {
Copy link

Choose a reason for hiding this comment

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

이 스타일도 이곳에 존재하면 안뒵니다.
Container 라는 추상화된것이 CardPassword 라는 구체적인것을 알고 있는 잘못된 상황인 것 입니다.

Copy link
Author

Choose a reason for hiding this comment

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

이름도 Input이면서... 왜 Container 밑에다가 두었을까요... 🥲 부끄럽습니다... Container 컴포넌트의 재사용성을 개선하면서 Container의 스타일시트도 함께 정리하였습니다! 감사합니다 :)

📌 관련커밋: f96d892


Text.defaultProps = {
className: '',
color: '#575757',
Copy link

Choose a reason for hiding this comment

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

텍스트의 기본값 컬러로 채택되기엔 다소 의아한 색상이군요.

Copy link
Author

@365kim 365kim May 3, 2021

Choose a reason for hiding this comment

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

이번 미션을 위한 figma 시안에서 텍스트 색상으로 가장 많이 등장하는 해당 색상(#575757)을 기본값으로 채택했었습니다. 당시 이 값을 정하기 위해 컴포넌트의 기본값이 무엇이 되어야 하느냐에 대해서 페어와 고민을 했었습니다.

기본값을 무엇으로 할까 라는 고민은 일반적으로 얘기하는 컴포넌트의 재사용성은 범위가 어디까지일까라는 의문으로 이어졌습니다.

컴포넌트 재사용성의 범위가 이 프로젝트에 국한된다면, figma 시안에서 가장 많이 사용된 #575757을 기본값으로 지정해도 될 것이라고 생각했습니다. 반면, 컴포넌트의 재사용성이 일반적으로 한 프로젝트에만 국한되는 것이 아니라 여러 프로젝트를 아울러 재사용할 수 있어야 한다는 개념이라면 어느 프로젝트에서도 범용적으로 기본 색상이 되는 #000과 같은 색상코드를 기본값으로 주어야 (혹은 아예 생략) 한다고 생각했습니다.

딱히 어느 쪽이 정답에 가까운지 모른 채 컴포넌트 재사용성의 범위에 대해 고민을 이어가던 중, 한 크루가 같은 디자인 시스템을 적용하는 다수의 프로젝트를 컴포넌트의 재사용 범위로 보고 작성하고 있다는 의견을 주었습니다. 저는 이 답변이 설득력이 있다고 생각했습니다. 그리고 이 경우에는 해당 프로젝트와 다른 프로젝트이더라도 동일한 디자인시스템이라면 텍스트 컬러를 #575757 과 같이 다소 특이한(?) 색상이어도 컴포넌트를 활용할 수 있을테니 기본값으로의 역할에 걸맞지 않을까 하는 생각으로 이 색상을 넣게 되었습니다…!

아직 컴포넌트 재사용성 범위에 대한 의견이 바뀌지 않아, 해당 기본값을 .Text { Color: #575757; } 과 같이 컴포넌트의 기본 스타일링에 남겨두었습니다…! 혹시 제가 잘못 생각한 부분, 놓친 부분이 있다면 피드백해주시면 감사하겠습니다….! 🙇🏻‍♀️🙇🏻‍♀️🙇🏻‍♀️

Copy link

@wow9144 wow9144 May 4, 2021

Choose a reason for hiding this comment

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

아주 중요한 과정들을 다 거치셨네요 👍
페어와 고민도 했고, 누군가(접니다) 말하는 것을 무작정 적용하는게 아니라 생각해보고 다시 고민해보고 또 다른 누군가와 이야기를 해보고 가장 설득력있다고 생각한대로 의견이 모아지고 그대로 해보는것.
무엇이 옳으냐가 중요한게 아니라 지금 해왔던 그 과정이 중요합니다.
그렇게 적용해보고 돌아보고 좋은 선택이었다고 생각되면 스스로에게 칭찬 한 번 해주고 좋지 못한 선택이었다면 깨닫고 배우면서 그렇게 한 발 한 발 성장하는 것입니다.
절대 타인의 의견을 자신의 생각 없이 그대로 받아들이시면 안됩니다! 👍 👍

같은 디자인 시스템을 적용하는 다수의 프로젝트 가 매우 설득력이 있지만, 제 의견은 여전히 텍스트 컬러가 특이한 색인것을 동의하기 힘듭니다 :)
그 이유로
이름이 Text입니다. 도 아니고 !,
도 아니고 !!,
도 아니고 !!!
이것은 마치 html 의 기본 태그중 하나인 <p> 태그의 color가 #575757 인것과 같게 느껴집니다.
#575757이 많이 쓰이는것은 그저 지금 이 프로젝트에서 많이 쓰이는 색상이 우연히 #575757 일뿐인것이지, 만약 지금 이 프로젝트에서 가장 많이 쓰이는 텍스트가 다 볼드 처리 되어 있고 색깔이 빨간색이라면 그럼 Text라는 이름의 컴포넌트의 디폴트컬러를 red로 font-weight는 bold 로 할 것이냐는 물음이 따를것 같습니다.
만약 이 물음에도 여전히 yes 라는 답이 나오면 그 때는 저도 동의하겠습니다.
(컨셉 그 자체에는 동의하지 않지만 일관성이 있으므로 이해하고 적용 할 수 있으니까 동의)

제 의견은 여기까지 하겠습니다 :)
어디까지나 이것은 제 개인의 의견일뿐이므로 저에게 설득을 당하시든지 마시든지 어떤 선택을 하시더라도 저는 마상을 입지 않으니걱정 안하셔도 됩니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

성장에 대한 좋은 말씀 감사합니다! 앞으로도 명심하며 학습해나가겠습니다! 💪

제가 장기적으로 진행되는 프로젝트 경험이 없어서 좁은 시야로 생각했던 것 같습니다. 말씀해주신 내용을 비추어보면, 현재 시점에서는 가장 많이 사용하지만 특이한 색상을 기본 색상으로 지정함으로써 생기는 문제가 있을 것으로 예상되는데, 제가 아직 컴포넌트 사용 경험도 적고 기능 확장이나 디자인 변경을 겪어보지 못해서 구체적으로 어떤 문제가 있을지 상상하기는 어려운 같습니다...!

그래서 우선 컴포넌트가 Text보다 조금 더 구체적인 이름을 갖게 하거나 기본값을 설정하기보다 primary 옵션을 가질 수 있게 하는 쪽으로 고민해보겠습니다! 좋은 피드백 주셔서 감사드립니다 😆


return (
<div>
<Text className="AddCompletePage__Text" fontSize="1.5rem">
Copy link

Choose a reason for hiding this comment

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

src/pages/AddCardPages/AddCompletePage/style.css

.AddCompletePage__Text {
margin: 7rem 0 5rem;
font-size: 1.5rem;
}

로 하지 않고 fontSize를 굳이 props로 전달해야 하는 이유가 있을까요?
Text 컴포넌트가 fontSize를 받을 수 있도록 구현되어 있기 때문에?
이 컴포넌트에서 Text 의 fontSize가 몇 인지 한 눈에 아는게 어떤 가치를 가질 수 있을까요?
AddCompletePage__Text 라는 이름으로 스타일링이 되어 있다는것만으로 이미 충분한 가치를 가질 수 있지 않을까요?

재사용 가능한 컴포넌트는 재사용 할 수 있기 때문에 좋기도 하지만 많은 곳에서 의존을 하고 있기 때문에 위험성도 올라갑니다.
수정이 일어났을 때 사용중인 모든곳에 영향을 미치기 때문에 한 번에 다 바꿀 수도 있지만, 한 번에 전체 박살(?)을 낼 수도 있습니다 :)
그리고 재사용 가능한 컴포넌트의 핵심은 그저 많은 곳에서 사용을 하게 하는게 아니라, 많은곳에서 내부 구현을 몰라도 사용을 '편하게' 할 수 있는게 더 중요합니다.
많은 곳에서 사용하라고 만들어놨지만 그것을 사용하는게 오히려 더 복잡성을 늘리게 할 수도 있고 불편하게 할 수도 있고 굳이 사용해야 할 의미가 없을 수도 있습니다.
그런 의미에서 components 안에 있는 컴포넌트들이 과연 재사용 가능한가(어디에서나 그 이름 그대로 범용적으로 쓸 수 있고, 쓰는게 더 편하고, 내부 구현을 몰라도 되는가) 차원에서 고민을 좀 더 해보시면 좋을 것 같습니다.
이 내용은 원래 별도 코멘트로 작성하려고 했으나 여기가 예시로 들기에 좋은 것 같아서 여기에 작성합니다.

Copy link
Author

@365kim 365kim May 3, 2021

Choose a reason for hiding this comment

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

처음 컴포넌트를 작성할 때, className을 받는 것이 스타일링을 custom하기 편하다는 점, style은 굳이 props로 명시해서 받지 않아도 어디서든 inline으로 넣어줄 수 있다는 점을 간과한 채 컴포넌트를 작성했었습니다.

필요한 스타일 속성이 생길때마다 Text 컴포넌트에 textAlign, color, 등등 컴포넌트에도 하나하나 추가해주면서 사용하다보니 이건 뭔가 아니다 싶었고 그제서야 className을 받을 수 있게 변경했었습니다. 요기의 fontSize는 미처 정리되지 못한 삽질의 잔재인 것 같습니다... 🥲🥲

컴포넌트의 선언부와 사용부를 구분하여 생각하고, 사용부에서 컴포넌트를 범용적으로, 그리고 편리하게 사용할 수 있도록 변경하였습니다. fontSize와 같이 미리 prop으로 지정되어있어야만 사용할 수 있는 구조를 다 걷어내고, 커스텀 스타일링은 className을 활용하거나 필요한 경우 style={{fontSize: 1.2rem}}과 같이 사용하는 방향으로 변경하였습니다!

📌 관련커밋: 6a366a1

Copy link

Choose a reason for hiding this comment

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

👍

import cvcImage from '../../../../../images/cvc.png';

export const SecurityCodeInput = (props) => {
const { securityCode, setSecurityCode, passwordInputRef } = props;
Copy link

Choose a reason for hiding this comment

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

보안코드 입력 컴포넌트가 passwordInputRef 를 알고 있어야 하는게 좀 의아했는데 코드를 찾아가보니 포커스를 다음으로 변경하기 위해서 그랬군요.
보안코드의 관심사는 passwordInputRef 가 아니라 포커스를 다음으로 이동 시킬 ref 이기 때문에 네이밍을 좀 더 추상화 해주는게 좋을 것 같습니다.

실무의 상황으로 예를 들자면 카드 비밀번호는 이 화면에서 언제든 사라지고 다른 페이지로 가거나 보안 키패드 모달이 떠서 그쪽으로 이동 되거나 할 수 있고, 보안코드 다음에 입력될 인풋이 다른게 오게 되거나 순서가 바뀌거나 할 수 있습니다.
지금처럼 SecurityCodeInput 내부에서 passwordInputRef 라는 이름을 쓰게 되면 위와 같은 상황이 생겼을 때 이 이름도 변경해야 하지만, 지금 이름을 적절히 추상화해두면 SecurityCodeInput 를 사용하는측에서 전달하는 ref 만 바꿔주면 되기 때문에 변화에 대응하기가 쉬워집니다.
포커스 이동과 관련하여 다른 컴포넌트도 그렇지는 않은지 함께 보시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

지금이야 제 머릿속에 카드번호 다음 > 만료일 다음 > ... 이렇게 순서가 바로 떠오르지만, 10일만 지나도, 아니 한 5일만 지나도 왜 passwordInputRefsecurityCodeInput이 받지? 라는 생각을 할 것 같습니다... 🥺

카드번호 입력 컴포넌트가 받고 있던 expirationDateInputRef,
만료일 입력 컴포넌트가 받고 있던 ownerNameInputRef,
보안코드 입력 컴포넌트가 받고 있던 passwordInputRef,
모두 refToBeFocusedNext 로 보내주게 변경하였습니다...!

컴포넌트의 사용시점을 고려하듯, 미래에 코드를 볼 사람, 혹은 제가 1개월 후에 이 코드를 본다면? 이라는 상황을 가정해서 작성했다면 좀 더 네이밍을 잘할 수 있었을까 싶네용,,, 각 객체의 관심사에 맞도록 이름을 지어주기, 쉽지는 않겠지만 계속해서 의식하면서 훈련해나가겠습니다! 좋은 피드백 감사드립니다 😆

Copy link

Choose a reason for hiding this comment

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

하루님 말씀을 들으니 문득.....
제가 모기업 면접 때 코드를 어떤 마음가짐으로 작성하는가? 같은 질문을 받았었는지, 그냥 제가 마지막으로 하고 싶은 말로 했었는지 정확히 기억이 나지는 않지만...
'내가 지금 작성하는 코드는 일주일 뒤, 한 달 뒤, 1년 뒤의 나에게 보내는 편지' 라는 말을 했던 기억이 떠오르는군요.. 그 땐 ㅋ ㅑ 하면서 자아도취에 빠졌었는데 지금 생각해보니 오글 거리네요 -_-;;
면접 때 한 번 써먹어보세요ㅋㅋ 저는 좋은 결과가 있었던것으로 기억합니다ㅋㅋ

Copy link
Author

Choose a reason for hiding this comment

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

이 댓글 보고 주변 크루들도 면접에서 꼭 써먹어야겠다고 하네요 ㅋㅋㅋㅋ 저도 응용해서 꼭 써먹어 보겠습니다! 좋은 결과 생기면 리뷰어님 덕분입니다 😆👍

maxLength={SECURITY_CODE_LENGTH}
value={securityCode}
/>
<Button theme="question-mark" type="button">
Copy link

Choose a reason for hiding this comment

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

Button 컴포넌트가 만들어지는 시점과 만들어진 이 컴포넌트가 사용되는 시점은 다릅니다.
(지금은 하루님이 만들고 하루님이 사용하니까 같은것 같지만, 실무에서는 내가 오늘 오전에 버튼을 만들고 오후에 쓰고 열흘뒤에 팀원이 내가 만든 버튼을 쓰고 하는 식으로, 컴포넌트의 선언부와 사용부의 코드가 작성되는 시점이 다릅니다.)
따라서 버튼의 텍스트는 사용하는 시점에 확정되고, theme props 에 의한 구현은 사용하기 전에 선언 시점에 확정 됩니다.
헌데 지금 Button의 모습을 보면 theme="question-mark" 이고 text는 ?로 둘이 완전히 매칭되고 있습니다.
이것은 재사용 가능한 Button이 아닙니다.
열흘뒤에 팀원인 제가 이 버튼을 사용 할 때 저는 확인, 제출하기, 계산 하기, 다음 등등.... 버튼을 사용하기 위해 수많은 텍스트를 넣을수 있을텐데, 받을 수 있는 theme 를 제가 직접 만들어야 하는건지(이건 재사용이 아닙니다.) ...
'재사용 가능한'의 의미는 구체적인것을 모르는 '범용적'이라고 생각하시고 설계 하시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

컴포넌트의 선언 시점과 사용 시점이 다른 것이 일반적이라는 말씀이 컴포넌트를 작성하는데 큰 힌트가 되었습니다! 저도 이해할 수 있을만큼 쉽고 자세하게 설명해주셔서 감사합니다...! 🙏

제가 작성한 컴포넌트를 제가 컴포넌트를 사용하면서도 '이 컴포넌트가 이런 props를 받아주고 있었나? 보내도 되나?' 싶어서 매번 컴포넌트를 확인하고 사용하곤 해야해서 그 짧은 기간동안에도 아주 별로인 사용성을 체험할 수 있었습니다...(좋은 경험이었습니다...🥲 )

현재는 theme 과 같이, 추가하려면 컴포넌트 사용부와 선언부 모두를 수정해야하는 props를 제거하고, className를 받을 수 있도록 변경하였습니다. 모든 컴포넌트가 통일성있게 className를 받아주고 있다보니 굳이 컴포넌트를 다시 열어보지 않아도 바로바로 스타일 작성 후 클래스 이름만 보내면 되서 좋은 것 같습니다...! 별 것 아닐 수 있지만 원래 작성했던 방식이 너무 경직되고 불편했어서 지금이 한결 편하게 느껴집니다 😆

📌관련 커밋: 398f431

Copy link

Choose a reason for hiding this comment

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

매번 클래스네임을 전달해야 한다면 컴포넌트 자체가 가진 디자인이 너무 아무것도 없을 수 있으니 사용성이 또 떨어질 수도 있습니다.
컴포넌트 자체가 완성된 디자인을 가지고 있고, props 로 옵션만 살짝 살짝 바꿔줄 수 있는 형태로 하는게 베스트인데....
그게 쉽지 않습니다.
그래서 확장 할 수 있도록 길을 열어두고 당장 필요한 최소한의 기능만을 가진 컴포넌트를 만들고 서서히 발전시켜가는식으로 합니다.
컴포넌트만 미리 다 만들어놓고 이걸로 조립 조립 하는 상황은 ... 그걸 하나의 프로젝트로 할 수 있는 여유가 있는 회사여야 해서 쉬운 경험이 아니기도 합니다.

);
}

function BackwardButton() {
Copy link

Choose a reason for hiding this comment

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

BackwardIcon 이라는 형태의 컴포넌트가 size, color를 props로 받도록 했다면 그것이 바로 재사용 가능한 컴포넌트가 됩니다.

Copy link
Author

Choose a reason for hiding this comment

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

제안해주신 BackwardIcon을 컴포넌트로 생성했습니다! svg의 path속성이 size와 연동되서 size를 입력하면 비례해서 커지도록 만들어보았습니다. (참고: MDN svg 튜토리얼)

처음에 BackwardButton만 따로 컴포넌트로 분리했는데 BackwardButton 보다 말씀해주신 BackwardIcon이 더 재사용성이 높을 것 같아서, Icon으로 변경해두었습니다...!

📌관련커밋: 398f431, fe95865

@365kim
Copy link
Author

365kim commented May 3, 2021

📌 리뷰반영 요약표

번호 리뷰내용 관련코멘트 반영커밋
1 컴포넌트 - 범용성 ✅
👉컴포넌트를 어디서나 사용할 수 있도록
Input 컴포넌트 재사용성 개선
바로가기 ab1db8d
Container 컴포넌트 재사용성 개선
바로가기 f96d892
Modal 컴포넌트 재사용성 개선
바로가기 37be358
2 컴포넌트 - 편리성 ✅
👉복잡도를 낮춰서 컴포넌트 사용자가 편하도록
Text 컴포넌트 재사용성 개선
바로가기 c57b0e2
3 컴포넌트 - 낮은 결합성 ✅
👉사용 시 내부 구현을 몰라도 되도록
Button 컴포넌트 재사용성 개선
바로가기 398f431
4 불필요한 모듈 분리는 관리비용만 ++
👉handler.js 삭제
바로가기 b5c2d97
5 라우팅을 삼항연산자로 하는 것은 nonsense!
👉react-router 적용
바로가기 ab9bce7
6 컴포넌트 관심사에 맞는 네이밍으로!
👉 refToBeFocusedNext로 변경
바로가기 f07e749

@wow9144 wow9144 merged commit f52c96a into woowacourse:365kim May 4, 2021
@wow9144
Copy link

wow9144 commented May 4, 2021

오랜만에 논쟁(?)을 했더니 재미 있네요. :)
다음 단계도 계속 기대하겠습니다 👍

@365kim
Copy link
Author

365kim commented May 9, 2021

자프님 안녕하세요!

1단계 머지해주신지 벌써 1주일이 다되어가네요...!
다음 단계 리뷰 요청이 많이 (진짜 쫌 많이...) 늦어질 것 같아서 댓글이라도 남겨봅니다,,,🙇🏻‍♀️

제가 기간 내 미션을 완수하지 못해서 코치님들께 질문을 드렸었는데 지난 미션을 완료하지 못했더라도, 새로 시작한 미션을 최우선으로 진행하는게 좋겠다라는 가이드라인을 내려주셨습니다. 개인적으로 이번 레벨 통틀어서 꾸준히 🥲 미션이 밀리고 있어서 현실적으로 방학이 시작하는 6월 5일 이후 부터 2단계 코드 작성을 시작할 수 있을 것 같습니다,,, 죄송합니다,,,🙏

지금 진행하는 미션에서 열심히 학습해서 쪼꼼 더 렙업한 2단계 코드로 인사드려보겠습니다!!! 💪 💪 💪

정성스런 피드백 남겨주셔서 다시 한 번 감사드립니다! 💙

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.

None yet

3 participants