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단계 - 행운의 로또미션] 하루(김하루) 미션 제출합니다. #8

Merged
merged 42 commits into from Apr 19, 2021

Conversation

365kim
Copy link

@365kim 365kim commented Apr 16, 2021

Jbee님, 안녕하세요!

오랜만에 인사드리는 🙋🏻‍♀️ 하루(@365kim) 입니다.
지난 레벨1 때도 로또 리뷰를 도와주셨었는데 요번에도 로또 미션으로 인사드리게 되었네요 ㅎㅎ
저는 주모(@jum0)과 함께 페어프로그래밍을 진행했습니다.


🎟 1단계 데모 페이지

✅ 구현 기능 목록

  • 리액트 제한사항
    • 클래스 컴포넌트를 사용한다.
  • 구입금액 입력
    • 로또 구입 금액을 입력하면, 유효성 검사 결과를 실시간으로 표시한다.
      • 최소 화폐단위(1원) 미만의 자릿수가 포함된 경우
      • 로또 1장의 가격(1,000원) 미만일 경우
      • 로또 1장의 가격(1,000원)으로 나누어 떨어지지 않을 경우
  • 로또 발급 및 확인
    • 로또 구입 금액을 제출하면, 자동구매로 금액에 해당하는 로또를 발급한다.
    • 번호보기 토글 버튼을 클릭하면 복권번호를 볼 수 있다.
  • 당첨번호 발표
    • 로또 발급 후 카운트 다운(3초)후에 이번주 당첨번호와 결과확인 버튼이 표시된다.
  • 당첨결과 확인
    • 결과 확인하기 버튼을 누르면 구매한 복권의 당첨여부 및 수익률을 모달로 표시된다.
      • 로또 당첨 금액은 고정되어 있는 것으로 가정한다.
    • 다시 시작하기 버튼을 누르면 초기화 되서 다시 구매를 시작할 수 있다.

💪 새롭게 시도한 부분

  • 에어비앤비에서 개발한 애니메이션 관련 라이브러리 로티를 사용해보았습니다.
    • props를 전달해서 애니메이션 속도 등을 조정할 수 있어서 gif보다 활용도가 좋다고 생각했습니다.

💪 UX/UI고려사항

  • 최초 앱 실행 시 구매금액 입력 창에 자동포커스 되도록 하였습니다.
  • 로또 단위금액으로 나누어떨어지지 않는 구매를 요청한 경우에도, 에러 메세지 없이 바로 구매를 진행했습니다. (거스름돈 처리)
  • 당첨번호 발표 시, 당첨결과 확인 시 애니메이션 효과를 추가해서 재미 요소를 추가했습니다.
  • 사용자에게 해당사항이 없는 통계는 과감히 생략하고 본인이 구매한 로또의 채점결과가 보이도록 했습니다.
  • 당첨결과에서 사용자가 입력한 번호 매칭여부를 CSS로 표시했습니다.
  • 로또번호를 공모양으로 스타일링 하면서도 텍스트 색상과 배경 색상간의 명도 대비를 웹표준(4.5 이상)울 준수했습니다.

리액트 공식문서를 채 다 못 읽은 채 미션을 진행하고 있어서 많이 부족한 상태지만, 크고 작은 개선사항 코멘트로 전해주시면 열심히 배우고 또 열심히 개선해보겠습니다..!!! 🙇🏻‍♀️🙇🏻‍♀️
감사합니다 😆

365kim and others added 19 commits April 13, 2021 16:00
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: 365kim <365kim@users.noreply.github.com>
Co-authored-by: 365kim <365kim@users.noreply.github.com>
Co-authored-by: 365kim <365kim@users.noreply.github.com>
Co-authored-by: 365kim <365kim@users.noreply.github.com>
Co-authored-by: 365kim <365kim@users.noreply.github.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: JUNMO HAN <wnsah052@naver.com>
Co-authored-by: 365kim <365kim@users.noreply.github.com>
</div>
<section className={`display-section ${this.state.isToggled && 'toggle'}`}>
{this.props.lottoBundle.map((v, i) => (
<Lotto numbers={v} key={String.fromCharCode(97 + i)} />
Copy link
Author

Choose a reason for hiding this comment

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

질문드립니다! 🙋🏻‍♀️ - 배열의 key값

index가 아닌 어떤 key를 사용할 수 있을지 고민스러웠습니다...🤔
페어와 고민하다가0, 1, 2, ...'a', 'b', 'c', ...로 변환해봤는데 괜찮은 방법일지 혹은 추천해주시고 싶으신 더 일반적인(?) 방법이 있을지 궁금합니다.

Choose a reason for hiding this comment

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

'key를 왜 넣어줘야 하는가?' 를 먼저 생각해보셔도 좋을 것 같네요?!

Copy link
Author

Choose a reason for hiding this comment

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

리액트에서 리스트를 만들 때 고유한 key를 설정하도록 강제하는 이유는 배열의 요소 추가, 수정, 삭제에 효율적으로 대응하기 위함으로 알고 있습니다.

  • 효율성 측면
    • 예시: [a b c] 리스트에서 a를 삭제하여 [b c] 를 렌더할 경우,
    • 문제: key값을 주지 않으면 a자리에 b를, b자리에 c를, c자리에는 아무것도 렌더하지 않는 번거로운 작업이 필요합니다.
      (key값을 주면 [a b c]에서 바로 a만 key값으로 잡아와서 빠르게 삭제할 수 있습니다.)
  • 데이터 맵핑 측면
    • 예시: [a b c] 리스트에서 a앞에 z를 추가하여 [z a b c] 를 렌더할 경우,
    • 문제: key값을 주지 않으면 리액트는 index를 default key값으로 활용하는데 이는 a(기존 index 0)가 가지고 있던 데이터가 z(새로운 index 0)에 맵핑되어 버립니다.(참고한 글)

두 가지 측면을 모두 생각해보았을 때 고유한 key값을 가져야 한다는 의미는 배열 내 형제끼리 중복되지만 않으면 된다라는 것 뿐만 아니라, 리렌더 되었을 때도 자신만의 고유한 key를 유지할 있어야 한다는 의미로 이해했습니다. 🤔 제가 이해한 내용이 맞다면, index를 순차적으로 변환해서 'a', 'b', 'c', ...를 key값으로 주는 방식은 위의 데이터 맵핑이 꼬이는 문제를 전혀 해결할 수 없다고 생각됩니다. 따라서 리액트에서 필요로하는 key값을 주려면 nano id와 같은 id 생성기를 이용해야겠다고 생각했습니다.

그런데 같은 글에 따르면, 지금 제 경우처럼 리스트가 변하지 않고 정적일 것이 보장(재정렬, 필터되지 않음)되는 경우인데 딱히 쓸만한 id를 가진 것도 아닌 경우에는 굳이굳이 id를 생성하기 보다는 index를 key로 사용해도 좋다고 하고 있습니다...! 제가 생각해도 key가 꼭 필요한 상황이 아니어서 이런 경우에는 key값을 생략하려고 합니다..!

혹시 제가 잘못 생각한, 잘못 알고 있는 부분이 있다면 지적해주시면 감사하겠습니다 🙇🏻‍♀️

Choose a reason for hiding this comment

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

@365kim 365kim changed the base branch from main to 365kim April 16, 2021 18:33
Copy link

@JaeYeopHan JaeYeopHan left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~ 코멘트 몇개 남겼어요!

기존 로또 프로젝트를 옮겨오지 말고 리액트 관점에 맞춰서 재정의하는 것이 더 좋을 것 같다는 생각이 들어요.
컴포넌트는 무엇인지, 어떤 단위로 나누어야 하는지도 같이 고민해봐요!

src/components/Animation.js Outdated Show resolved Hide resolved
src/components/PurchaseLotto.js Outdated Show resolved Hide resolved
src/components/PurchaseLotto.js Outdated Show resolved Hide resolved
src/components/PurchaseLotto.js Outdated Show resolved Hide resolved
src/components/PurchaseLotto.js Outdated Show resolved Hide resolved
src/components/WinningResult.js Outdated Show resolved Hide resolved
src/components/WinningResult.js Outdated Show resolved Hide resolved
src/components/WinningResult.js Outdated Show resolved Hide resolved
src/components/WinningResult.js Outdated Show resolved Hide resolved
@365kim
Copy link
Author

365kim commented Apr 18, 2021

리뷰어님 안녕하세요! 코멘트 남겨주신 것 보고 리팩토링 진행해봤습니다...! 💪


컴포넌트 설계 ✍🏻

✨컴포넌트✨는 비단 리액트에서뿐만 아니라 프로그래밍 전반에서 재사용이 가능한 최소 단위를 뜻하는 용어로 쓰인다고 알고있습니다.

이번 PR 에서 총 4개의 컴포넌트를 만들었는데, 이 컴포넌트 들은 최상위 App 컴포넌트를 쪼개기 위한 컨테이너 정도의 역할을 하고 있었습니다. 재사용 가능한 컴포넌트가 0개 였습니다...! (뜨헉)

재사용 가능한 컴포넌트를 만들자는 목적으로 리팩토링을 시작했습니다. 🚀 최상위 컴포넌트에 가까울수록 도메인 로직이 많이 들어가서 재사용할 수 있게 만들기 어렵지만, 하위 컴포넌트로 내려오면 좀 더 재사용 가능한 컴포넌트를 뽑아내기 쉬울 것이라고 생각했습니다.

그래서 우선, 컨테이너(containers) 역할을 하는 컴포넌트와 재사용 가능한(shared) 컴포넌트를 분리하는 방향으로 폴더 구조를 변경하였습니다.

./containers는 App 바로 밑의 큼직 큼직한 컴포넌트를, ./shared에는 재사용되는 컴포넌트를 배치했습니다. 컨테이너가 너무 크기 때문에 분리하려는 목적으로 만든, 재사용되지 않는 컴포넌트의 경우에는 해당 컨테이너 하위에 별도의 모듈로 분리했습니다. 덧붙여, 각 컨테이너에서 필요한 로직을 수행해주는 service.js를 추가했습니다.

한편, 앱 내에서 여러 컴포넌트에 걸쳐 사용되고 있다면 도메인과 연관이 있더라도 ./shared에 배치했습니다. 이름 상으로는 <PlainButton /> <ToggleButton /> 등 모든 버튼이 <Button />을 상속받게 해야할 것 같은데 그 안의 필요한 속성과 스타일이 서로 다 달라 보여서, 상속 받는게 어떤 의미가 있을지 모르겠다고 생각했습니다. 그래서 <Button />을 따로 만들지 않고 그대로 두게 되었습니다.

수십 개의 파일을 수정하면서 나름 대공사를 거쳤는데, 목적대로 재사용성이 높아진건지 잘 모르겠...습니다...🥲 분명 이전의 4개의 왕컴포넌트 보다는 낫겠지만... 한번 보시고 피드백 주시면 감사하겠습니다...!! 🙇🏻‍♀️🙇🏻‍♀️

components
┣ App 
┃
┣ containers
┃ ┣ PurchaseForm
┃ ┣ UserLotto
┃ ┃ ┗ Lotto
┃ ┃ WinningNumbers
┃ ┃ ┗ WinningNumberList
┃ ┗ UserResult
┃   ┣ ResultSummary
┃   ┗ ResultTable
┃  
┗ shared
   ┣ Animation
   ┣ PlainButton
   ┣ SubmitButton
   ┣ ToggleButton
   ┣ XButton
   ┣ LottoBall
   ┗ TwoDigitBal

@365kim
Copy link
Author

365kim commented Apr 18, 2021

📌 리뷰반영 요약표

번호 리뷰내용 관련코멘트 반영커밋
1 컴포넌트 단위 다시 생각해보기!
👉 폴더구조 변경 및 재사용 가능한 컴포넌트 생성
바로가기 4ff87ad
2 배열의 key값 넣어주는 의미 다시 생각해보기!
👉 재정렬 되지 않는 배열에는 기본값인 index를 key값으로 활용
바로가기 30b056d
3 props 스마트하게 전달받기!
👉 <Lottie /> props 스프레드로 전달
바로가기 46f94bd
4 더 깔끔하게 메서드 작성!
👉 reduce 활용, let 제거
바로가기 1ee3b01 be11367
5 태그 선택에 유의!
👉 적절하지 않은 <section> 태그 대신 <p> 태그 적용
바로가기 4551a99
6 스타일링은 class로 제어!
👉 BEM 네이밍 적용
바로가기 e2f6bb1
7 더 적합한 네이밍으로!
👉 getStatistics -> getComputedResult으로 변경
👉shouldPlayAnimation -> isLoading 으로 변경
바로가기 911aaf7, a35218f

@365kim 365kim requested a review from JaeYeopHan April 18, 2021 15:00
export default class TwoDigitBall extends Component {
render() {
const { number, className } = this.props;
const numberText = number < 10 ? `0${number}` : number;

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.

앗 내장 메서드가 있었군요...! 🥺 바로 적용해보겠습니당!! 감사합니다..!! 🙏

const { number, className } = this.props;
const numberText = number < 10 ? `0${number}` : number;

return <span className={`TwoDigitBall ${className}`}>{numberText}</span>;

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.

오와오!!!!! 대박스 바로 설치해서 적용해보겠습니다! 제이비님 꿀벌이셨군요,,, 꿀팁에서 꿀이 쥬르르...🍯💙

365kim and others added 2 commits April 19, 2021 00:12
Co-authored-by: Jbee <ljyhanll@gmail.com>
Co-authored-by: Jbee <ljyhanll@gmail.com>
Comment on lines 9 to 12
<p className="ResultSummary__profit">
<span className="ResultSummary__description">당첨 금액</span>
<span className="ResultSummary__value">{profit}원</span>
</p>
Copy link

Choose a reason for hiding this comment

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

저라면 요 단위를 컴포넌트화 해볼 것 같아요!
Record라는 이름으로요!
ResultSummary라는 컴포넌트가 재사용될까요? 🤔

Suggested change
<p className="ResultSummary__profit">
<span className="ResultSummary__description">당첨 금액</span>
<span className="ResultSummary__value">{profit}</span>
</p>
function Record() {
return (
<p className="record">
<span className="record-label">{label}</span>
<span className="record-value">{children}</span>
</p>
)
}

Copy link
Author

@365kim 365kim Apr 18, 2021

Choose a reason for hiding this comment

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

오어... 이거였습니다... 제가 하고싶었던게,,,🤩 이렇게 누가봐도 재사용할 수 있을 것 같은 컴포넌트였는데... 현실은...🥲ResultSummary는 상위 컨테이너가 장황해져서 쪼개려는 목적으로 분리하게 되었습니다,,, 위에 공유해주셨던 챠크라의 컴포넌트 천천히 살펴보고, 단순히 쪼개는 것보다 재사용성을 좀 더 고민하면서 이렇게 재사용 가능한 컴포넌트를 늘려보겠습니다!! 정말정말 시야가 트이는 느낌이에요! 감사합니다~🙇🏻‍♀️😆🙇🏻‍♀️

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

Choose a reason for hiding this comment

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

1단계 제한사항이 클래스 컴포넌트를 사용하는 것이라서 클래스 컴포넌트로 반영해보았습니다..!

📌 관련커밋: 61ff42a

Copy link
Author

Choose a reason for hiding this comment

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

앗 코멘트를 이제봤네요...! 높은 응집도 + 낮은 결합도 유념해서 도전해보겠습니다!! 🚀🚀🚀🚀 감사합니다! 💙

Comment on lines +11 to +20
const { className, children, ...props } = this.props;
const buttonClass = cx({
Button: true,
[`${className}`]: true,
});

return (
<button className={buttonClass} {...props}>
{children}
</button>
Copy link

Choose a reason for hiding this comment

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

  1. children 그대로 넘겨주면 되겠는데요?
  2. className에 아무 것도 안 넘긴다면?

+) classname엔 대문자가 좀 어색하네요 ㅎㅎ

Suggested change
const { className, children, ...props } = this.props;
const buttonClass = cx({
Button: true,
[`${className}`]: true,
});
return (
<button className={buttonClass} {...props}>
{children}
</button>
const { className, ...props } = this.props;
const buttonClass = cx(className, 'Button');
return (
<button className={buttonClass} {...props} />

Copy link
Author

Choose a reason for hiding this comment

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

앗 조건부 아니고 항상 넣어줄거면 심플하게 인자로 넣어주면 되는거였군요! 리드미에 Syntax 다시 읽고왔습니당ㅎㅎ 감사합니다!

classNames('foo', 'foo', 'bar'); // => 'foo bar'
classNames('foo', { foo: false, bar: true }); // => 'bar'

Copy link
Author

@365kim 365kim Apr 19, 2021

Choose a reason for hiding this comment

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

  1. children 그대로 넘겨주면 되겠는데요?

말씀해주신 것처럼 children...props에 한꺼번에 넣을 경우 JSX가 어떻게 처리되는지 아직 머릿 속에 잘 안그려져서 Babel에 적어봤습니다..!

// children을 children 자리에 직접 넣어주는 경우
<button className={buttonClass} {...props}>
  {children}
</button>
// 트랜스파일 후
export default class Button extends Component {
  render() {
    const { className, children, ...props } = this.props;
    const buttonClass = cx(className, "Button");
    return /*#__PURE__*/ React.createElement(
      "button",
      _extends(
        {
          className: buttonClass
        },
        props
      ),
      children  // 👈  요기 들어감
    );
  }
}
// children을 ...props에 포함시켜넣거나 props와 같은 위계(?)에서 넘겨주는 경우
<button className={buttonClass} children={children} {...props} />
// 트랜스파일 후
export default class Button extends Component {
  render() {
    const { className, children, ...props } = this.props;
    const buttonClass = cx(className, "Button");
    return /*#__PURE__*/ React.createElement(
      "button",
      _extends(
        {
          className: buttonClass,
          children: children  // 👈  요기 들어감
        },
        props
      )
    );
  }
}

두 경우에 트랜스파일링 후 children이 들어가는 위치가 다르게 보이고, React.createElement API에서 세번째 인자를 children로 넣어주고 있어서 두번째 인자에 한번에 ...props로 보내도 괜찮을까 싶었는데 실행시켜 보니까 잘되네요,,,,?,,,,,👉👈

요것이 이해가 안되는 건 제가 JS문법 기초가 부족해서일까요,,,🥲 어떻게 이해하면 좋을지 조언해주시면 매우매우 감사하겠습니다,,, 🙏

Copy link
Author

@365kim 365kim Apr 19, 2021

Choose a reason for hiding this comment

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

  1. className에 아무 것도 안 넘긴다면?

className에 아무것도 안넘긴다면 undefined: true가 되고 있었군요...! 수정해주신대로 하면 classnames가 알아서 처리해주니 문제 없겠네요! 감사합니다 😆

Copy link
Author

@365kim 365kim Apr 19, 2021

Choose a reason for hiding this comment

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

+) classname엔 대문자가 좀 어색하네요 ㅎㅎ

classnames은 정말 별 생각없이(...) 제 VS Code에 설치된 코드 스펠링 체크해주는 익스텐션 cspell에서 밑줄 띄워줘서 classNames으로 적었었는데요,,,! 필요할 경우 user dictionary에 추가하거나 cspell.json에서 설정을 작성자가 필요한대로 바꿀 수 있다는 사실을 알게되었습니다! (참고문서) classnames외에도 추가할 단어들이 있을 것 같고, 이 설정 파일도 push해놓는게 좋을 것 같아서 cspell.json에서 관리해보려고 합니당! 에디터에 cspell때문에 PROBLEM 몇십 개 줄줄이 뜨곤 했는데...(못본 척했지만...🥲) 이제 없앨 수 있게되었네요~~~😆

Copy link

@JaeYeopHan JaeYeopHan left a comment

Choose a reason for hiding this comment

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

수고하셨어요!ㅎㅎ
일부러 빡세게 리뷰했는데, 너무 빡빡했다면 피드백 해주세요!

코멘트 하나 살짝 추가해봤어요~ (https://github.com/woowacourse/react-lotto/pull/8/files#r615501370)

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