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

[3단계 - 행운의 로또 미션] 하루(김하루) 미션 제출합니다. #62

Merged
merged 116 commits into from
Mar 2, 2021

Conversation

365kim
Copy link

@365kim 365kim commented Feb 28, 2021

Jbee님, 안녕하세요!

우당탕탕 3단계를 만들어온 🙋🏻‍♀️ 하루(@365kim) 입니다.
아쉽게도 벌써 마지막 단계 리뷰요청을 드리게 되었습니다...! 🥲


요약

  • 데모  : 로또미션 3단계 - 하루
  • 구현 순서: 테스트 전체 작성 후, 각 테스트를 통과하기 위한 단위 기능 구현
  • 구현 결과: 작성한 cypress 테스트 모두 통과 ✔
    git clone https://github.com/365kim/javascript-lotto.git
    cd javascript-lotto
    git checkout 365kim-step3
    npm install
    npm run cypress
    

3단계 - 구현기능 목록

  • UI를 직접 구현한다.

  • 정상적인 로또 구입금액을 투입하면 로또 발급 단계로 넘어갈 수 있다.
    • 로또 발급버튼, 자동/수동 매수, 로또용지 추가버튼이 표시된다.
    • 전체 자동구매를 기본값으로 한다.
  • 로또용지를 추가해서 수동구매를 할 수 있다.
    • 용지추가 버튼을 누를 때마다 로또번호 선택용지가 한 장씩 추가된다.
    • 자동구매 수량이 0매이면 용지추가 버튼이 비활성화 된다.
    • 로또용지에서 6개 보다 적게 고르면 상태메세지가 화면에 표시된다.
    • 로또용지에서 6개 보다 적게 고르면 로또 발행버튼이 비활성화 되어있다.
  • 각 로또용지 별로 적용수량을 조정할 수 있다.
    • 새로 추가한 로또용지의 적용수량의 기본값은 1매이다.
    • 자동구매 수량이 남아있을 경우, 적용수량을 조정해서 수동구매로 전환할 수 있다.
    • 로또용지 삭제버튼을 누르면 해당 로또용지가 사라지고 해당 적용수량은 자동구매로 전환된다.
  • 로또복권 발행 버튼을 누르면 로또를 발행을 진행한다.
    • 자동구매 매수, 수동구매 매수가 적힌 안내메세지를 Confirm으로 표시한다.
    • '예'를 선택할 경우에만 로또복권을 발행한다.

UI구현 중 느낀 점 📝

  • 3단계에 추가되는 노드만 HTML 문서에 무작정 추가하는 식으로 진행했습니다. 이 방식은 제한적인 디자인 밖에 나오지 않고 전체적인 통일성을 놓치기 쉽기 때문에 지양해야 한다고 느꼈습니다.
  • 구매금액을 먼저 정하기보다 자동, 수동을 자유롭게 추가하고 해당 금액을 결제하는 흐름이 더 자연스럽게 느껴졌습니다. '수동 구매 후 남는 금액이 있다면 자동으로 구매할 수 있어야 한다.'는 요구사항이 수정되어도 좋을 것 같습니다. 👉👈
  • UI 참조 레퍼런스 (웹 & 앱)

365kim and others added 30 commits February 16, 2021 15:01
@365kim 365kim changed the base branch from main to 365kim February 28, 2021 13:16
src/js/components/App.js Outdated Show resolved Hide resolved
src/js/layouts/selectPaper.js Outdated Show resolved Hide resolved
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.

코멘트 남겼습니다! 🙇

index.html Outdated Show resolved Hide resolved
src/js/layouts/selectPaper.js Outdated Show resolved Hide resolved
src/js/layouts/selectPaper.js Outdated Show resolved Hide resolved
src/js/components/PurchaseAmountInput.js Outdated Show resolved Hide resolved
src/js/components/PurchaseOptionInput.js Outdated Show resolved Hide resolved
src/js/components/PurchaseOptionInput.js Outdated Show resolved Hide resolved
src/js/constants/appStages.js Show resolved Hide resolved
src/js/model/LottoPaper.js Outdated Show resolved Hide resolved
src/js/model/appStageManager.js Show resolved Hide resolved
src/js/components/App.js Outdated Show resolved Hide resolved
@365kim
Copy link
Author

365kim commented Mar 1, 2021

📌 리뷰반영 요약표

번호 리뷰내용 관련코멘트 반영커밋
1 AppStageManager 역할 분리, LottoManager추가 바로가기 07438ae
2 validate 함수 내 거스름돈 계산 부분 분리 바로가기 07438ae
3 setStates 내 view제어 부분 분리 바로가기 07438ae
4 get'Nth'ElementRemoved 유틸함수 개선 바로가기 6b1646c
5 잘못 사용한 summary 태그 수정 바로가기 61975ae
6 키보드조작 가능한 UI로 수정 바로가기 f3385f0
+ constructor에서도 초기값으로 할당하기 때문에 문제가 되는 것...?! 바로가기

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.

수고하셨습니다! 👏
정리 + 자문자답 인상적이네요 ㅋㅋㅋ

@JaeYeopHan JaeYeopHan merged commit e785c83 into woowacourse:365kim Mar 2, 2021
@365kim
Copy link
Author

365kim commented Mar 23, 2021

3/23(화) 오전수업 - 셀프코드리뷰

내가 설계한 구조를 지금도 다시 설명할 수 있는가?

  • 네!
  • App.js 에서 AppStageManager와 LottoManager를 생성하고 각 컴포넌트에 주입합니다.
  • 각 컴포넌트는 자신과 연관된 AppStage에 대해 구독합니다.
  • AppStage가 변경되면 AppStageManager가 각 컴포넌트에게 이를 알립니다.
  • 컴포넌트는 다음과 같이 구성됩니다.
    • [1] 구매금액 입력
    • [2] 구매옵션 입력
    • [3] 구매한 티켓표시
    • [4] 당첨번호 입력
    • [5] 당첨결과 표시


한 단계 더 추상화해서 분리할 수 있는 부분은 없는가?

  • 현재 모든 Observable Subject를 AppStageManager를 하나로 관리하고 있는데, 공통된 메서드를 추출해서 Subject 클래스를 만들고, 이를 extends해서 사용하면 Observer 클래스의 재사용성이 더 좋아질 것 같습니다.


상수화가 적절히 이루어졌는가?

  • appStage.js에 앱의 단계를 200, 300 과 같은 상태코드로 표현했었는데 이 부분은 지금 봐도 다른 분들이 읽으시기에 띠용스러울 수 있을 것 같습니다. Value 값을 그냥 ‘appInit’과같은 문자열로 수정해도 좋을 것 같습니다.
  • 렌더 함수에서 CSS class를 조작할 때 replace(‘text-green’, ‘text-red’)와 같이 class이름을 직접 넣어주고 있습니다. 이 class의 이름을 상수화해서 관리하면 혹시 모를 오류를 예방하는데 도움이 될 것 같습니다.


예외를 위한 고민이 적절한가?

  • 유틸함수에서 예외 처리가 하나도 안되고 있습니다..! 예를 들어 target이 없을 때 target.classList로 접근하면 런타임 에러가 날텐데 아무 처리 없이 사용하고 있습니다. 이 부분이 보완되면 좋겠습니다. 


네이밍이 아쉬운 부분은 없는가?

  • getNthElementRemoved, getNthElementRemoved와 같은 유틸함수의 이름이 조금 아쉽습니다. lodash에서는 nth(array, n)와 같은 함수를 제공하는데, 이를 보면 ‘nth’라는 워딩만으로도 충분히 Array의 Element라는 의미가 전달된다는 것을 알 수 있습니다. 따라서 removeNth, updateNth 로도 충분히 유틸함수의 기능을 표현할 수 있겠습니다.


한 함수에서 한 가지 기능만 하고 있는가?

  • 이벤트 핸들러들은 공통적으로 많은 일들을 수행하고 있습니다. 해당 이벤트에 대한 핸들링이긴 해서 역할에는 맞다고 생각되는데 이게 단일 책임을 지키고 있는 것인지 헷갈립니다,,,

웹 접근성 부분에서 개선할 부분은 없는가?

  • 당첨결과 모달창에서 다시 시작하기 버튼은 tab으로 접근이 가능한데, 그냥 닫기 버튼[x]는 접근이 불가능합니다. 닫기 버튼도도 tabbable하게 수정되면 좋겠습니다..

@365kim
Copy link
Author

365kim commented Apr 25, 2021

학습로그

HTML <form>태그

태그

  • form, input, button

@365kim
Copy link
Author

365kim commented Apr 27, 2021

학습로그

입력폼 UX/UI

태그

  • UX, UI, 입력폼

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.

3 participants