Skip to content

[2단계 - 웹 기반 로또 게임] 에프이(박철민) 미션 제출합니다.#333

Merged
liswktjs merged 41 commits intowoowacourse:chysisfrom
chysis:step2
Mar 11, 2024
Merged

[2단계 - 웹 기반 로또 게임] 에프이(박철민) 미션 제출합니다.#333
liswktjs merged 41 commits intowoowacourse:chysisfrom
chysis:step2

Conversation

@chysis
Copy link
Member

@chysis chysis commented Mar 4, 2024

안녕하세요 샐리! 미리 말씀드렸던 것처럼 주말에 미션을 진행하지 못해 아직 완성되지 않은 상태입니다. 빠르게 완성시켜 나가겠습니다 :)

구현 사항

step2-index

  • 메인 컨트롤러 역할을 하는 파일입니다.
  • step1과 마찬가지로 구매 금액을 입력받고 WebLottoController로 흐름을 넘깁니다.

WebLottoController

  • 구매 금액을 step2-index로부터 받아서 이후 프로그램 실행 흐름을 담당합니다.
  • 로또 구매 개수와 로또 번호 목록을 동적으로 생성해서 html에 추가합니다.
  • 당첨 번호와 보너스 번호를 입력받는 form을 렌더링합니다.
  • 당첨 번호와 보너스 번호 입력을 담당하는 이벤트 리스너와 핸들러를 추가합니다.
  • 모달창을 화면에 띄웁니다(미완성).
    • 모달창은 hide 클래스로 관리하는데, visible 속성값으로 보이거나 보이지 않게 하고 있습니다.

Object

  • 프로그램에서 사용되는 html 요소들을 querySelector로 불러와서 파일로 따로 저장했습니다.

반응형 웹

  • vw, vh 값을 사용해서 여러 해상도에서 동일한 화면 비율을 유지합니다.
  • 프로그램 인터페이스의 크기(가장 바깥 사각형)도 지금은 반응형으로 구현했는데, 이 크기는 고정값으로 사용하는 것이 사용자 경험 측면에서 더 좋을지 고민됩니다.

에러 처리

  • 구매 금액 유효성 검사를 통과하지 못한 경우 해당 에러 메시지를 화면에 출력하고 다시 입력 받을 수 있게 합니다.
  • 아직 당첨 번호와 보너스 번호 에러 출력은 작동하지 않아서, 추후 완성하려 합니다.

step1 구조와의 통일성

  • step1의 구조와 거의 같은 구조로 step2를 작성할 수 있었습니다. controller와 view의 구조만 바꼈을 뿐, 도메인 로직의 수정이 없었습니다.

아직 기능 완성도 제대로 되지 않은 상태라 프로그램 구조도 깔끔하지 못한 것 같습니다. 빠르게 기능을 완성하고, 리팩토링 과정을 거치려 합니다 :)

감사합니다.

Copy link

@liswktjs liswktjs left a comment

Choose a reason for hiding this comment

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

안녕하세요 에프이
2단계 미션도 수고 많으셨습니다 👍

css를 변수화하셔서 깔끔하게 관련 관심사 별로 잘 묶어 주셨더라고요!
다음에 디자인 시스템 등을 구현할 때도 꼼꼼하게 잘 하시지 않을까 라는 생각이 들었습니다

내일이 또 다른 미션이 시작되는 만큼,
부담가지시지 말고 천천히 반영과 기능 구현해서 리뷰 요청해주세요 🙏

다음 미션 맛집 🍱 파이팅입니다!

index.html Outdated
<div id="lotto-app">
<span id="lotto-app-logo">🎱내 번호 당첨 확인🎱</span>
<form id="purchase-amount-form">
<label>구입할 금액을 입력해주세요.</label>
Copy link

Choose a reason for hiding this comment

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

label은 input과 연결을 해주게 되면 유저의 사용성을 더 높여줄 수 있습니다!
한 번 더 관련 키워드를 찾아보시면 좋을 것 같아요!

https://developer.mozilla.org/ko/docs/Web/HTML/Element/label

Copy link
Member Author

Choose a reason for hiding this comment

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

보조 기술 사용자 또는 터치 스크린을 사용하는 사용자까지 고려할 수 있다는 점을 알게 되었습니다. 만약 설명 문구와 같이 input과 직접적인 관련이 없는 텍스트라면 별도로 연결하지는 않았습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

<label>당첨 번호</label>
  <div id="winning-numbers-inputs">
    <input type="number" class="winning-number-input" required />
    <input type="number" class="winning-number-input" required />
    <input type="number" class="winning-number-input" required />
    <input type="number" class="winning-number-input" required />
    <input type="number" class="winning-number-input" required />
    <input type="number" class="winning-number-input" required />
  </div>

위의 경우처럼 하나의 label과 여러 개의 input이 연결되어야 하는 경우에 서로 구별되는 id값을 부여해야 하는데 비효율적일 것 같다는 생각이 듭니다. 특히 input의 개수가 훨씬 많아진다면 더 그럴 텐데, 샐리는 이런 경우 어떻게 해결하시는지 궁금해요!

Choose a reason for hiding this comment

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

input의 개수가 많아질 때는 auto focus를 활용하곤 합니다!
auto focus에 대해서 한 번 방법을 찾아보시면 좋을 것 같아요

Choose a reason for hiding this comment

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

참고로 react에 넘어가서는 useRef를 사용해서 구현하기도 합니다 (아직 이르지만...소근소근 미리 알려드리는 팁입니다)

Copy link
Member Author

Choose a reason for hiding this comment

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

꿀팁까지 주셨는데 너무 감사합니다!!😆 관련 방법을 찾아봐야겠어요

Comment on lines +14 to +16
reset(target) {
target.innerHTML = '';
},
Copy link

Choose a reason for hiding this comment

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

자식 컴포넌트를 모두 비울때 innerHTML을 써도 좋지만
innerHTML은 XSS공격에 취약할 수 잇습니다!

replaceChildren 등의 방법을 한 번 활용해보시면 어떨까요?

Comment on lines +26 to +29
const lottoList = this.#lottery.map(lotto => {
return `<li><span>🎟️</span>${lotto.numberList.join(CONFIG_FORMAT.JOIN_SEPARATOR)}</li>`;
});
WebOutputView.printMessage(purchaseLottoList, lottoList.join(''));
Copy link

Choose a reason for hiding this comment

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

innerHTML로 해주면 편하긴 하지만 다른 코멘트에서 언급한 것 처럼 xss공격에 취약할 수 있습니다!
createElement를 다음 미션부터는 적용해서 사용해주시면 좋을 것 같아요 👍

@chysis
Copy link
Member Author

chysis commented Mar 11, 2024

안녕하세요 샐리! 기다려주셔서 감사합니다.

점심 뭐 먹지 미션과 함께 진행 중인데, 로또 미션의 도메인 로직이 더 많고 유기적으로 연결되어 있어서 더 어려운 것 같습니다.

*️⃣배포 주소

https://chysis.github.io/javascript-lotto/dist/

*️⃣구조

Components

  • 로또 게임 결과 모달을 구성하는 요소들을 템플릿 리터럴로 반환합니다.
  • 당첨 번호와 보너스 번호를 입력하는 폼을 템플릿 리터럴로 반환합니다.

WebLottoController

  • 게임의 전체 흐름을 담당합니다.
  • 이전에 구매 금액을 step2-index에서 관리했다면, 이번에는 controller에서 관리합니다.

step2-index

  • 시작 파일로, 필요한 파일들을 import하고 controller를 실행합니다.

Views

  • InputView에서는 사용자가 입력한 값을 유효성 검사를 거쳐 읽어들입니다.
  • OutputView에서는 컴포넌트를 동적으로 렌더링하거나, 화면에 문구를 출력하는 역할을 합니다.

Objects

  • 프로그램 중간에 값을 업데이트해야 하는 요소가 아니라면 Object에서 한 번만 선언해두고 import해서 사용합니다.

*️⃣기능 구현 사항

#293

이전 step1 PR에 달아주신 코멘트에 답변과 추가 질문을 함께 남겼습니다!

*️⃣고민한 내용

  • 이번 미션을 하면서 반응형 UI를 설계하기 위해 다양한 시도를 했습니다. 데스크탑과 노트북 두 가지 환경에서 작업을 했는데요, 노트북에서는 페이지를 80%로 축소한 상태였기 때문에 요소가 메인 컨테이너 밖으로 나가는 것을 인지하지 못하고 있었습니다. 내부 요소들은 px값으로 크기를 설정한 반면에 외부 컨테이너만 vw, vh값으로 설정하려고 하는 과정이 매끄럽지 못한 것 같습니다. 이를 fit-content 속성을 활용해서 해결했습니다.
  • Controller와 View를 잘 분리하기 위해 고민했습니다.

*️⃣질문

  • HTML 요소를 런타임에 동적으로 추가한 뒤, 추가된 요소에 event listener를 추가하려고 할 때 렌더링이 된 이후에 추가된다는 보장이 없는 것 같습니다.

    • 예시입니다
    const child = `<button id="button">제출</button>`;
    container.insertAdjacentHTML('beforeend', child);
    
    const addedChild = document.querySelector('#button"); // 위에서 추가된 버튼
    addedChild.addEventListener('click', ()=>{...});
    • 이런 경우를 해결하기 위해 어떤 키워드를 공부하면 좋을지 추천 부탁드립니다!

감사합니다 🙂

Copy link

@liswktjs liswktjs left a comment

Choose a reason for hiding this comment

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

안녕하세요 에프이
맛집 미션 중에 로또 미션까지 수고많으셨습니다 👍

HTML 요소를 런타임에 동적으로 추가한 뒤, 추가된 요소에 event listener를 추가하려고 할 때 렌더링이 된 이후에 추가된다는 보장이 없는 것 같습니다.

말씀해주신대로, dom에서 가져오는 element가 꼭 존재할 것 이라는 보장을 할 수 없습니다
그렇기 때문에 dom이 없는 경우에 대해서 분기처리를 진행해주면 좋습니다
예를 들어

const addedChild = document.querySelector('#button");
if(addedChild === null){
    throw Error('not found')
}
return addedChild

이런식으로 null인경우 에러를 뱉고 아닌 경우 element를 뱉는 함수를 만들어 찾지 못하는 경우에 대해서 처리할 수 있습니다!
이를 util화 하셔서 앞으로 남은 미션들에서도 사용해보시면 좋을 것 같습니다 👍
타입스크립트와 함께 사용하면 element들에 대한 타입들도 지정해줄 수 있습니다

1단계 pr과 더불어 2단계 pr에도 코멘트 남겼으니 한 번 확인 부탁드립니다 :)

Comment on lines +23 to +32
const purchaseAmountInput = $('#purchase-amount-input');
const winningNumbersForm = $('#winning-numbers-form');
const lottoResultModalSection = $('#lotto-result-modal-section');

purchaseAmountInput.value = '';
purchaseAmountInput.focus();
WebOutputView.reset(purchaseResult);
WebOutputView.reset(purchasedLottoList);
WebOutputView.reset(winningNumbersForm);
lottoResultModalSection.classList.add('hide');

Choose a reason for hiding this comment

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

initApp 하나로 묶어도 크게 상관은 없지만
가능하다면 init하는 element끼리 함수로 묶어서 호출해주면 가독성이 더 올라갈 것 같습니다
예를 들어 init 함수에서 호출되는 함수명의 네이밍 만으로 어떤 것들을 init시켜주고 있는지 파악하기 쉬울 수 있습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

코드의 가독성을 조금 더 신경쓰겠습니다!

Comment on lines +38 to +42
const purchaseAmount = WebInputView.readPurchaseAmount(e);
if (!purchaseAmount) return;
this.#purchaseAmount = purchaseAmount;
this.#lottery = new LotteryMachine(this.#purchaseAmount).makeLottery();
this.#showPurchasedResult(this.#purchaseAmount);

Choose a reason for hiding this comment

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

addEventListener안에 넣어주는 함수를 따로 분리해주는 것도 좋을 것 같습니다

renderLottoResult(rankList, profit) {
const modalContainer = $('#lotto-result-modal-container');
modalContainer.replaceChildren();
modalContainer.insertAdjacentHTML('beforeend', ResultModal(rankList, profit));

Choose a reason for hiding this comment

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

제가 알기로는 insertAdjacentHTML 해당 문법또한 xss공격에 취약하다고 알고 있습니다
현재 미션에서는 xss 공격을 막기위한 최선은 방법은 createElement이니
번거롭더라도 createElement를 사용해서 구현하면 좋을 것 같습니다 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

createElement를 사용하면 코드가 길어질 텐데, 관련 유틸 함수를 만드는 것도 고려해보겠습니다!


width: calc(70vh * 0.57);
height: 70vh;
width: fit-content;

Choose a reason for hiding this comment

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

현재 구입할 금액을 입력하면 화면이 커지고 있더라고요 👀
저의 리뷰가 에프이를 더 헷깔리게 한 것 같네요 😅 죄송합니다
다른 코멘트에도 적었지만, 안에 자식 요소들의 width들이 고정되어 있는 경우에 fit-content를 사용하면 Padding값도 지정이 되어 있기 때문에 자연스럽게 width를 차지할 수있을 거라고 생각했습니다!
다만, 현재와 같은경우에는 자식 요소들을 %로 지정해주고 있기 때문에 부모에 min-width나 width로 값을 고정해주고 자식 요소들을 %로 지정해주는 게 적절할 것 같네요
다시 한 번 혼란을 드려 죄송합니다

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 코드를 충분히 파악하지 못한 것 같습니다😅 UI를 설계할 때 크기를 어떻게 지정할 것인지도 먼저 고려하는 습관을 가져야겠어요 :)

@liswktjs liswktjs merged commit 0ff949c into woowacourse:chysis Mar 11, 2024
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