-
Notifications
You must be signed in to change notification settings - Fork 168
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단계 - 행운의 로또 미션] 지그(송지은) 미션 제출합니다. #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존의 틀을 해치지 않는 선에서 나름대로 UI를 만들어서 제작해보았습니다.
피드백 기다리겠습니다! 감사합니다
@@ -25,29 +32,32 @@ export default class LottoController { | |||
} | |||
|
|||
reset() { | |||
this.isAutoPurchase = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
페이지 로드 시 처음 purchase type은 default로 auto가 되도록 설정했습니다.
src/js/LottoController.js
Outdated
purchaseManually() { | ||
this.manualInputView | ||
.show() | ||
.init(this.purchasedPrice / LOTTO_NUMBERS.LOTTO_UNIT); | ||
this.manualInputView.on('submitNumbers', e => | ||
this.inputManualNumbersHandler(e.detail) | ||
); | ||
this.manualInputView.on('confirmAll', e => | ||
this.confirmManualPurchaseHandler(e.detail) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수동구매 선택 시 실행됩니다.
src/js/LottoController.js
Outdated
inputManualNumbersHandler(eventDetail) { | ||
const ticketNumbers = eventDetail.numbers.map(manualNumber => | ||
Number(manualNumber.value) | ||
); | ||
if (!isUniqueManualNumber(ticketNumbers)) { | ||
alert(ALERT_MESSAGES.DUPLICATE_NUMS); | ||
return; | ||
} | ||
this.createManualLottos(ticketNumbers, eventDetail.ticketNumber); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/js/LottoController.js
Outdated
confirmManualPurchaseHandler(manualCount) { | ||
if (manualCount < this.purchasedPrice / LOTTO_NUMBERS.LOTTO_UNIT) { | ||
const agreeAutoPurchase = confirm(ALERT_MESSAGES.CONVERT_TO_AUTO_ALERT); | ||
if (agreeAutoPurchase) { | ||
this.manualInputView.hide(); | ||
this.purchaseAutomatically( | ||
this.purchasedPrice / LOTTO_NUMBERS.LOTTO_UNIT - manualCount | ||
); | ||
} | ||
} else { | ||
this.manualInputView.hide(); | ||
this.purchasedLottosView.show(); | ||
this.purchasedLottosView.renderLottos(this.lottos); | ||
this.winningResultView.show(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수동 구매를 선택하였으나 숫자를 (구입 티켓의 수만큼) 모두 입력하지 않은 경우, confirm 창으로 자동구매 전환을 안내합니다.
동의 시 자동 구매로 전환되고 남은 티켓의 개수만큼 로또를 자동으로 구매합니다.
const newLottos = Array.from({ length: lottoCount }, () => { | ||
return new Lotto(); | ||
}); | ||
this.lottos = [...this.lottos, ...newLottos]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수동 구매 후 남는 금액으로 자동 구매를 하는 경우도 있기 때문에 스프레드 연산자를 사용하여 this.lottos
에 새로운 lotto들을 추가하는 방식으로 바꾸었습니다.
src/js/views/ManualInputView.js
Outdated
} | ||
|
||
createManualLottos() { | ||
const lottoTicekts = [...Array(this.count)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의미 없는 index for문을 사용하는 것보다 이렇게 스프레드 연산자를 활용하여 map
이나 forEach
를 돌리는 것이 낫다고 배웠는데,
정확하게 이 문법으로 작성하는 것인지를 까먹었습니다 😭 마땅한 키워드가 떠오르지 않아 검색하기가 어려워서 일단 작성해보았습니다.
지적 부탁드립니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for 문 보다 Array 의 메서드를 사용하는게 대부분의 경우에 더 낫습니다.
이 경우는
const lottoTickets = Array.from({length: this.count}, (_, idx) => `<li>....</li>`).join('')
이렇게 하시면 될 것 같네요 :)
src/js/views/ManualInputView.js
Outdated
Number($element.dataset.manualIndex) === | ||
LOTTO_NUMBERS.LOTTO_MANUAL_COUNT - 1 | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수동 구매 각 티켓의 마지막 숫자에서는 moveFocus
이벤트가 발생하지 않습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의도가 잘 드러나는 코드가 좋은 코드입니다.
이 댓글을 달아주신 이유가 만약 제가(혹은 이 코드를 보는 다른 누군가가) 이 코드를 더 잘 이해하도록(혹은 정확히 이해하지 못할까봐) 설명(마치 주석처럼)을 해주신것이라면, 코드 자체에서 그 설명이 표현되는게 좋습니다.
수동 구매
에서는 포커스
가 다음
으로 이동
된다.
마지막은 제외
한다
라는 의도를, 코드를 읽는 사람이 별도 설명 없이 코드만 보고 알아차릴 수 있다면 완벽한 코드입니다.
사용성을 위해 포커스를 다음으로 옮기시는것은 굉장히 좋습니다.
실무에서도 디자이너나 기획자가 따로 언급하지 않는 경우도 많은데 개발자가 사용성을 생각해서 고민하고 추가로 구현하는것은 굉장히 좋은 태도라고 생각합니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
간단하게 메서드를 따로 분리하여 네이밍해보겠습니다!
src/js/views/ManualInputView.js
Outdated
const allConfirmBtn = document.createElement('template'); | ||
const convertToAutoCaption = document.createElement('template'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createElement('template')
을 사용하여 아래에서처럼
this.$element.append(convertToAutoCaption.content, allConfirmBtn.content);
으로 작성하면 html을 붙일 수 있다는 것을 알게 되었는데, 처음 접한 방식이라 괜찮은 방식인지 궁금합니다! 🙄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createElement() 하면 DOM 엘리먼트를 만들어 낼 수 있으므로 해당 엘리먼트를 통해 DOM API를 이용할 수 있는 이점이 있습니다.
안타깝게도 template 엘리먼트가 IE 에서 지원되지 않으니 이점 추가로 알아두시면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IE 대응은 어렵군요... 😭 다른 방식으로 수정해보도록 하겠습니다!
src/js/views/ManualInputView.js
Outdated
bindConfirmEvent() { | ||
$('#manual-confirm-btn').addEventListener('click', () => { | ||
this.emit( | ||
'confirmAll', | ||
[...$$('.manual-wrapper > .lotto-detail')].length | ||
); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import View from './View.js'; | ||
import { $ } from '../utils/dom.js'; | ||
|
||
export default class PurchaseTypeSelectView extends View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용성을 올리기 위해 고민하신 흔적이 보여서 좋았습니다. 👍
다만 전체 프로세스가 진행되는 과정중에서 약간의 논리 오류가 발견 되었으니 코멘트 확인하시어 이 부분을 중점적으로 수정 하시면 될 것 같습니다.
프로세스의 각 단계를 나눠서 생각해보시면 수정 하시는데 도움이 될 것 같습니다.
용지에 번호를 적는 단계, 로또가 발행되는 단계, 발행된걸 확인하는 단계 등등..
화이팅!
src/js/views/ManualInputView.js
Outdated
const allConfirmBtn = document.createElement('template'); | ||
const convertToAutoCaption = document.createElement('template'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createElement() 하면 DOM 엘리먼트를 만들어 낼 수 있으므로 해당 엘리먼트를 통해 DOM API를 이용할 수 있는 이점이 있습니다.
안타깝게도 template 엘리먼트가 IE 에서 지원되지 않으니 이점 추가로 알아두시면 좋을 것 같습니다.
src/js/views/ManualInputView.js
Outdated
} | ||
|
||
createManualLottos() { | ||
const lottoTicekts = [...Array(this.count)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for 문 보다 Array 의 메서드를 사용하는게 대부분의 경우에 더 낫습니다.
이 경우는
const lottoTickets = Array.from({length: this.count}, (_, idx) => `<li>....</li>`).join('')
이렇게 하시면 될 것 같네요 :)
src/js/views/ManualInputView.js
Outdated
Number($element.dataset.manualIndex) === | ||
LOTTO_NUMBERS.LOTTO_MANUAL_COUNT - 1 | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의도가 잘 드러나는 코드가 좋은 코드입니다.
이 댓글을 달아주신 이유가 만약 제가(혹은 이 코드를 보는 다른 누군가가) 이 코드를 더 잘 이해하도록(혹은 정확히 이해하지 못할까봐) 설명(마치 주석처럼)을 해주신것이라면, 코드 자체에서 그 설명이 표현되는게 좋습니다.
수동 구매
에서는 포커스
가 다음
으로 이동
된다.
마지막은 제외
한다
라는 의도를, 코드를 읽는 사람이 별도 설명 없이 코드만 보고 알아차릴 수 있다면 완벽한 코드입니다.
사용성을 위해 포커스를 다음으로 옮기시는것은 굉장히 좋습니다.
실무에서도 디자이너나 기획자가 따로 언급하지 않는 경우도 많은데 개발자가 사용성을 생각해서 고민하고 추가로 구현하는것은 굉장히 좋은 태도라고 생각합니다 :)
src/js/LottoController.js
Outdated
|
||
createManualLottos(manualNumbers, ticketNumber) { | ||
const lotto = new Lotto(); | ||
lotto.inputManualNumbers(manualNumbers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inputManualNumbers 라는 메서드를 별도로 두기보다는 Lotto를 생성 할 때 numbers를 넘기는게 의미상으로도 더 좋을 것 같습니다.
넘기면 그 숫자로 로또를 생성하고(수동) 안넘기면 랜덤한 숫자로 생성하고(자동) ...
현재 구조라면 랜덤 숫자로 이미 생성된 로또에 뒤늦게 숫자를 덮어 씌우는듯 하네요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
덮어 씌우는 것이 맞긴 합니다 😅 그냥 넘어갔는데 짚어 주셔서 감사합니다!
다른 로직으로 수정해보도록 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇게 하면 Lotto
의 constructor에 분기를 나눠줘야 할 것 같아서 inputManualNumbers
를 넣은 것인데,
좀 더 고민 후 반영해보겠습니답
src/js/LottoController.js
Outdated
} | ||
|
||
confirmManualPurchaseHandler(manualCount) { | ||
if (manualCount < this.purchasedPrice / LOTTO_NUMBERS.LOTTO_UNIT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 의 조건에 다양한(다른) 연산이 섞여 있으면 가능하면 분리해주는게 좋습니다.
< 비교 연산과
/ 산술 연산이 같이 존재하므로
산술 연산을 하여 하나의 값으로 귀결 시켜 변수에 할당(변수의 이름을 통해 의미 부여)하고 manualCount 와 이 변수와 비교 연산을 하는식으로 2개의 다른 연산을 나누면 코드의 가독성이 올라갑니다.
src/js/LottoController.js
Outdated
alert(ALERT_MESSAGES.DUPLICATE_NUMS); | ||
return; | ||
} | ||
this.createManualLottos(ticketNumbers, eventDetail.ticketNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 시점에 createManualLottos 를 하는게 맞는지 한 번 고민해보실 필요가 있을 것 같습니다.
숫자를 쓰고 확정을 누른 시점은 로또 용지에 번호를 다 적은 상태일뿐 로또가 발행되는것은 '구매 확정하기'를 누른 뒤어야 하지 않을까 싶습니다.
실무에서 이것과 비슷한 예를 찾는다면 회원가입을 하기 위해 아이디를 입력하고 아이디 중복 검사를 눌렀는데 아이디로 회원가입이 되어버린것과 같은 상황이라고 볼 수 있습니다.
아직 비밀번호도 주소도 기타 다른 필드들이 입력되지 않았고 가입하기
버튼을 누르지도 않았는데 말이죠 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각각의 수동 로또 티켓에 확정을 누르면 Lotto
객체를 생성하여 번호들을 보여주는 로직을 간단하게 하려고 했던 건데,
말씀 주신 부분이 일리가 있는 것 같습니다. 좀 더 자연스러운 실행 흐름으로 바꿔보겠습니다!
궁금한점 답변 2단계에서 Lotto 모델이 너무 많은 일을 한다는 피드백을 바탕으로 LottoProcessor 클래스를 만들어서 당첨 결과를 계산하게끔 했는데, 해당 계산 담당 로직들을 class로 만들어 내부 메소드들로 구현한 것이 괜찮은 방식인지 궁금합니다! 각 게임마다 new 연산자로 각 view를 새로 만들어내는 것처럼, 계산하는 로직도 현재 로또 배열과 당첨 번호를 담고 있는 유일한 인스턴스라고 생각해서 그렇게 작성했는데, 어떻게 생각하시는지 피드백 받고 싶습니다. ManualInputView가 처음 생각보다 조금 길어진 것 같습니다. html 템플릿이 많아서 그런 것 같기도 한데, 자동 구매 시에는 해당 view가 필요하지 않기 때문에 index.html 파일에 미리 넣어 놓고 show, hide하기보다는 수동 구매 선택 시에만 ManualInputView를 append하는 방식으로 사용하고 있습니다. 이에 대한 리뷰 부탁드리고 싶습니다! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 주신 부분들 반영하여 수정해보았습니다.
좋은 피드백 감사드립니다 😆
src/js/views/ManualInputView.js
Outdated
` | ||
) | ||
.join(''); | ||
// const lottoTickets = Array.from({length: this.count}, (_, idx) => `<li>....</li>`).join('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실수로 올라갔네요 😂 다음 커밋에서 지웠습니다!
const manualForms = Array.from( | ||
{ length: this.count }, | ||
(_, idx) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array로 loop을 도는 로직을 말씀주신 대로 바꾸었습니다.
isLastManualNumber(manualIndex) { | ||
return Number(manualIndex) === LOTTO_NUMBERS.LOTTO_MANUAL_COUNT - 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
마지막 input에서는 moveFocus가 되지 않는 코드를, 명시적으로 설명하기 위해 함수로 한번 분리하였는데
말씀주신 의도대로 잘 이해한 것이 맞을까요? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 이정도만 되어도 가독성이 훨씬 나아진것 같습니다 👍
} | ||
|
||
showAllConfirmButton() { | ||
const allConfirmBtn = document.createElement('div'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IE 대응을 위해 createElement('template')
대신 createElement('div')
를 사용했습니다.
inputManualTicketHandler(eventDetail) { | ||
const ticketNumbers = eventDetail.numberInputs.map(input => | ||
Number(input.value) | ||
); | ||
if (!isUniqueManualNumber(ticketNumbers)) { | ||
alert(ALERT_MESSAGES.DUPLICATE_NUMS); | ||
return; | ||
} | ||
this.manualInputView.saveTemporaryTicket( | ||
ticketNumbers, | ||
eventDetail.ticketIdx | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수동 로또 1장 확정 시 로또를 바로 생성하지 않고, 확정된 숫자들의 목록으로만 보여지게끔( this.manualInputView.saveTemporaryTicket
) 로직을 수정했습니다.
this.purchaseAutomatically(lottoTotalCount - manualCount); | ||
} | ||
} else { | ||
this.createManualLottos(manualTickets); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수동 구매 시 전체 구매 확정을 눌렀을 때 createManualTickets
를 실행하도록 바꿨습니다.
this.initNumbers(); | ||
constructor(numbers) { | ||
this._numbers = new Set(numbers) || new Set(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자동일 때는 빈 Set
이 만들어지고 아래의 initRandomNumbers
를 실행하며,
수동일 때는 인자로 받은 숫자들로 로또를 생성합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3단계까지 완료 하시느라 수고 많으셨습니다.
앞으로 진행 될 다음 미션들도 계속 기대 할게요 화이팅! :)
isLastManualNumber(manualIndex) { | ||
return Number(manualIndex) === LOTTO_NUMBERS.LOTTO_MANUAL_COUNT - 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 이정도만 되어도 가독성이 훨씬 나아진것 같습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[3/23 셀프 리뷰]
while (this._numbers.size < Lotto.LOTT0_LENGTH) { | ||
this._numbers.add(getRandomNumber()); | ||
this._numbers.add(getRandomNumber(1, LOTTO_NUMBERS.LOTTO_MAX_NUM)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getRandomNumber
와 같은 유틸성 함수는 최대/최소값 인자를 받아서 여러 곳에서 사용할 수 있도록 작성하는 게 좋은 것 같다.
lotto.initRandomNumbers(); | ||
return lotto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return
문 위는 한 줄 띄는 게 가독성이 좋다.
} | ||
|
||
createAutoLottos(lottoCount) { | ||
const newLottos = Array.from({ length: lottoCount }, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
length만 알고 있는 Array
를 채우는 방식을 몇 번 더 연습해서 적재적소에 적용해봐야겠다.
LOTTO_NUMBERS.LOTTO_MANUAL_COUNT | ||
); | ||
this.bonusNumber = winningNumbers[LOTTO_NUMBERS.WINNING_NUMBER_COUNT - 1]; | ||
this.bonusNumber = winningNumbers[LOTTO_NUMBERS.LOTTO_MANUAL_COUNT]; | ||
this.rankCounts = Array(5).fill(0); | ||
this.earningRate = 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 파일의 이름은 LottoManager
등으로 했어도 좋을 것 같다.
그때도 리뷰어님께서 말씀 주셨는데, 까먹고 바꾸지 않았다 😅
export function getRandomNumber(min, max) { | ||
return Math.floor(Math.random() * max) + min; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
moveFocus($element) { | ||
if ($element.value.length === 2) { | ||
if (this.isLastManualNumber($element.dataset.manualIndex)) return; | ||
$element.nextElementSibling.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nextElementSibling
는 써놓고 또 잊고 있었음! 유용한 DOM selector 기억기억
$('#auto-btn').classList.toggle('btn-default'); | ||
$('#auto-btn').classList.toggle('btn-cyan'); | ||
$('#manual-btn').classList.toggle('btn-default'); | ||
$('#manual-btn').classList.toggle('btn-cyan'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순히 toggle
만 하다보면 리로딩이나 악의적인 css 변경 등 외부 변화에 취약하여 앱의 상태와 실제 UI가 맞지 않을 수 있기 때문에,
isAutoPurchase
bool 값에 따라 조건문 분기로 class의 추가/삭제를 해주는 것이 좋아 보인다.
@@ -5,11 +5,13 @@ export default class View { | |||
} | |||
|
|||
show() { | |||
if (!this.$element) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(이전 변경 내역이라 아래에 코멘트가 안 달린다.)
CustomEvent
를 여기서 선언해 놓고 class 자체를 상속 받아 사용했기 때문에
실제로 dispatchEvent
가 어디에 붙어 사용되는지, 왜 이곳에 붙여주는지 생각지 않고 사용했던 것 같다.
이번 지하철 노선도 미션에서 window.dispatchEvent
와 window.addEventListener
를 사용하며 조금 더 CustomEvent
의 작동 방식과 친해진 것 같다.
moveFocus($element, idx) { | ||
if ($element.value.length === 2) { | ||
if (idx === LOTTO_NUMBERS.WINNING_NUMBER_COUNT - 1) return; | ||
if (this.isLastWinningNumber(idx)) return; | ||
$$('.winning-number')[idx + 1].focus(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(이전 변경 내역이라 위에 코멘트가 안 달린다.)
bindNumberInputEvent
메소드에서
this.winningNumbers = [
...$('#winning-numbers-form').getElementsByTagName('input'),
].map(input => Number(input.value));
대신
this.winningNumbers = [
...$('#winning-numbers-form').elements
].map(input => Number(input.value));
이런 식으로 돌았어도 될 것 같다. form
요소 내부의 form
컨트롤이 되는 대상들을 모아보기!
<<<<<<< HEAD | ||
debug@^4.0.1, debug@^4.1.0, debug@^4.1.1: | ||
======= | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge conflict 시 yarn.lock
파일도 잘 보자 🤦♀️
학습로그객체 프로그래밍의 기본 (ref)
show/hide와 append (ref)웹페이지 렌더 과정
기타Array 내장 메서드 사용하기 (ref)const lottoTickets = Array.from({ length: this.count }, (_, idx) => `<li>....</li>`).join('')
|
안녕하세요! 마지막 로또 3단계 미션 제출합니다 😆
지그's 로또 데모
2단계가 머지되기 전에 3단계를 이미 진행하고 있어서, 2단계 머지 후 새로운 브랜치에서 기존 3단계 작업 내용들을 cherry-pick하여 가져왔는데
중간에 커밋이 올바르게 딸려오지 않은 부분들이 있을 수 있습니다. (전체 코드 상에서는 문제가 없지만, 각 커밋에 들어간 코드가 100% 내용을 반영하지 않는 부분도 있습니다.) 코드 상세 내용들 코멘트로 달아놓도록 하겠습니다!
✅ 3단계 TODO
기능
테스트
[ ] 검증: 1~45 사이의 숫자 입력 시 알럿이 뜬다.[ ] 자동 구매 취소 시 계속해서 수동 구매 번호 입력 폼을 채울 수 있다.🛠 컴포넌트 및 구조도
🗂 디렉토리 구조
🤔 고민한 점
🙄 궁금한 점
Lotto
모델이 너무 많은 일을 한다는 피드백을 바탕으로LottoProcessor
클래스를 만들어서 당첨 결과를 계산하게끔 했는데, 해당 계산 담당 로직들을 class로 만들어 내부 메소드들로 구현한 것이 괜찮은 방식인지 궁금합니다! 각 게임마다 new 연산자로 각 view를 새로 만들어내는 것처럼, 계산하는 로직도 현재 로또 배열과 당첨 번호를 담고 있는 유일한 인스턴스라고 생각해서 그렇게 작성했는데, 어떻게 생각하시는지 피드백 받고 싶습니다.ManualInputView
가 처음 생각보다 조금 길어진 것 같습니다. html 템플릿이 많아서 그런 것 같기도 한데, 자동 구매 시에는 해당 view가 필요하지 않기 때문에index.html
파일에 미리 넣어 놓고show
,hide
하기보다는 수동 구매 선택 시에만ManualInputView
를 append하는 방식으로 사용하고 있습니다. 이에 대한 리뷰 부탁드리고 싶습니다!그 밖의 내용들은 코멘트로 달았습니다 🤓