[2단계 - 웹 기반 로또 게임] 해온(백솔비) 미션 제출합니다. #218
Conversation
There was a problem hiding this comment.
안녕하세요 해온!
2단계 미션을 빨리 끝내셨네요! 고생하셨어요 😃
- 전체적으로 자바스크립트 코드와 구조는 깔끔해서 크게 떠오르는 피드백이 없었어요.
- BEM으로 클래스명을 네이밍하셨다고 하는데, BEM에 적절하지 않은 네이밍이 몇몇개 보여서 조금 아쉬웠습니다. 예를 들면, winning__container, total__budget, winning__number 등이 있겠네요.
기타 상세한 피드백은 각 코드 라인에 남겨두었으니 확인 후 답변 부탁드릴게요!
querySelector를 이용해 요소를 선택한 부분을 Dom.js 파일에 넣었는데 ( ex) export const budgetForm = $('.budget__form') ) 이 부분을 Element.js 파일을 따로 만들어서 분리하는 것이 더 좋을까요?? Dom.js에 적기에는 파일이 통일성 없어보이고 Element.js에 적기에는 너무 다 분리해놓은 느낌이라 고민됩니다...!
개인적으로는
export const $ = (selector) => document.querySelector(selector);
export const $$ = (selector) => document.querySelectorAll(selector);
export const open = ($target) => $target.classList.add('open');
export const close = ($target) => $target.classList.remove('open');여기까지만 utils/Dom.js에 있어도 좋지 않을까 생각해요. querySelector로 상수화한 요소들은 유틸리티의 성격과는 거리가 있는 것 같아요.
저는 utils에 있는 함수들은 다른 프로젝트에서도 사용할 수 있을 정도의 범용성을 가져야 한다고 생각해요. (그런 의미에서 open, close 함수들도 utils에 있어야 하는지 조금 의문이긴 하지만... 놓을 곳이 애매하니 여기에 두어도 괜찮을 것 같습니다.)
다른 이야기이지만 저는 개인적으로 querySelector를 통해서 가져온 DOM 요소를 별도로 상수화하는 것을 좋아하는 편은 아니예요. 나중에 DOM 요소가 추가되거나 삭제되면 해당 상수는 더 이상 사용할 수 없게 되는 경우가 많거든요.
그래서 저는 보통 $('.element')와 같이 바로 DOM에 접근하는 것을 선호하는 편이예요.
innerText와 textContent 중 어느것을 더 선호하시나요?? 둘의 차이점을 찾아봤는데 반환할 때의 차이점만 있더라구요. 저는 반환용으로 쓰는게 아닌데 어떤 걸 쓰는게 더 좋을지 궁금합니다!
어느 한 쪽에 선호가 있진 않고, 상황에 따라서 다르게 사용하는 것 같습니다.
둘은 반환할 때의 차이점 뿐만 아니라 다른 차이점도 있습니다.
둘의 차이점에 대해서는 MDN 문서를 참고해주시면 좋을 것 같습니다.
https://developer.mozilla.org/ko/docs/Web/API/Node/textContent#innertext%EC%99%80%EC%9D%98_%EC%B0%A8%EC%9D%B4%EC%A0%90
LottoDomSimulator의 retryProcess 메서드에서 4~7번째 줄은 다 화면에 대한 초기화입니다. LottoDomSimulator에서 해야될 것은 아닌거 같은데 retry button event가 있는 ResultView의 retryHandler 메서드 에서 하려니 거기에서 $totalBudget이랑 $ticketsList에 대한 초기화를 해줘도 되는지 모르겠습니다. (지금 찾아보니까 retryHandler 메서드에 $winningContainer를 가져와 쓰긴 했네요...) 이렇게 섞어쓰는게 맞는건지 의구심이 들어서 여쭈어봅니다...!
저는 LottoDomSimulator가 로또 게임을 실행하는 주체라는 점에서 retryProcess에서 어색한 점을 찾지 못했어요. 지금 코드도 좋다고 생각하는데요.
왜 LottoDomSimulator에서 해야될 것은 아니라는 생각이 드셨나요? 해온이 생각하시는 LottoDomSimulator의 역할은 무엇인가요?
src/css/style.css
Outdated
| --bold-extra: 700; | ||
| --bold-semi: 600; | ||
| --bold-bold: 500; | ||
| --bold-regular: 400; |
There was a problem hiding this comment.
보통 400은 regular, 500은 medium, 600은 semibold, 700은 bold라고 하니 참고 부탁드릴게요
| table { | ||
| border-collapse: collapse; | ||
| border-spacing: 0; | ||
| } |
There was a problem hiding this comment.
브라우저마다 제공하는 스타일이 다르기 때문에 모든 브라우저에서 동일하게 출력하기 위해서 reset을 했습니다!
| font-size: var(--size-body); | ||
| } | ||
|
|
||
| :root { |
There was a problem hiding this comment.
CSS에서 사용하는 값들에 대해서도 상수화를 진행해주셨네요. 좋습니다 👍
몇몇 개는 미사용된 것도 있는 것 같은데, 미사용된 값들을 그대로 두신 이유가 있으신가요?
There was a problem hiding this comment.
피그마에 있는 내용을 바탕으로 상수화 했는데 미사용한 값을 삭제하는 것을 깜박했네요...! 😅
src/css/style.css
Outdated
| } | ||
|
|
||
| /* etc */ | ||
| .text__center { |
There was a problem hiding this comment.
텍스트 중앙 정렬을 해주는 클래스에 대해서도 BEM을 사용해주셨는데요. text를 block, center를 element로 봐주신 걸까요? BEM의 형식과는 조금 맞지 않는 것 같아서요.
There was a problem hiding this comment.
무의식적으로 띄어쓰기도 다 __로 작성했네요...! text-center로 클래스명을 수정했습니다!
| width: 400px; | ||
| height: 600px; | ||
| margin: 0 auto; | ||
| padding: 3rem 1rem; |
There was a problem hiding this comment.
px은 너무 절대적이고 고정적인 느낌이고 em은 또 유연한 느낌이더라구요. 그래서 반응형 웹에도 사용 가능하고 부모 요소에 영향을 덜 받는
rem을 선호합니다. 여기서는 피그마에 있는 내용은 px로 되어있길래 그 부분만 px로 작성하고 나머지는 rem으로 작성했습니다! 차라리 다 통일하는게 더 좋을까요...?
There was a problem hiding this comment.
rem 단위로 무조건 통일하는 것보다 사용하는 목적에 따라서 적절하게 사용하는 것이 생각해요.
root의 font-size에 따라서 스타일링이 깨지지 않는다고 생각하면 px 단위를 그대로 사용하셔도 좋구요.
저는 지금도 나쁘지 않다고 생각합니다!
저 질문을 드렸던 이유는 rem 단위를 무조건적으로 적용하는 경우가 종종 있어서 (제가 그랬었습니다 😅) rem 단위를 사용하는 명확한 이유가 있으셨는지 궁금해서 여쭤본 것이었습니다. 해온은 이유가 있으셨으니까 좋습니다!
| this.budgetView.readEvent('inputPrice', (e) => this.budgetProcess(e.detail)); | ||
| this.winningView.readEvent('inputWinningNumber', (e) => this.winningNumberProcess(e.detail)); | ||
| this.resultView.readEvent('retryCommand', () => this.retryProcess()); |
There was a problem hiding this comment.
CustomEvent로 구입 금액 입력, 당첨 번호 입력, 다시 시작 입력을 감지할 수 있도록 한 구조가 깔끔하네요 👍
index.html
Outdated
| </div> | ||
| <div class="bonus__input"> | ||
| <label for="bonus__number">보너스 번호</label> | ||
| <input type="text" id="winning__number" class="bonus__number" /> |
There was a problem hiding this comment.
이곳은 id가 bonus__number가 아니라 winning__number인 이유가 있나요?
There was a problem hiding this comment.
label 요소의 for 속성에 bonus__number가 들어가 있어서 여쭤봅니다.
There was a problem hiding this comment.
원래 id가 bonus__number였는데 winning__number에 한번에 넣고 싶어서 바꿨습니다! 근데 lable의 for 속성도 바꿔야 하는데 깜빡 했습니다. 수정하겠습니다!
index.html
Outdated
| </section> | ||
| <section class="modal"> | ||
| <div class="modal__container"> | ||
| <button class="close__button"> |
There was a problem hiding this comment.
제 생각에는 button이 block, close가 modifier일 것 같아요. (button--close)
button이 가장 바깥쪽에 있는 상위 요소, close가 변형을 나타낸다고 생각하거든요.
혹시 close__button으로 네이밍하신 이유가 있으신가요?
(네이밍에 정답은 없어요! 단순히 이유와 의견이 궁금해서 여쭤보는 거예요!)
There was a problem hiding this comment.
맞네요! 그냥 단순히 닫는거니까 close__button이라고 해야지!라고 적었어요. close는 modifier에 해당하니까 맨 뒤로 가는게 맞는 거 같아요! modal이라는 block에 button이라는 element, close라는 modifier라고 해서 modal__button--close로 수정해보았습니다.
src/view/ResultView.js
Outdated
| close($winningContainer); | ||
| } | ||
|
|
||
| closeModal() { |
There was a problem hiding this comment.
closeModal은 실제로 modal을 닫는 함수의 이름처럼 보여서 조금 아쉬운 네이밍인 것 같습니다. bindRetryEvent와 같은 네이밍을 적용하는 건 어떠신가요?
There was a problem hiding this comment.
안그래도 modal을 받는다는 이유로 메서드명을 이렇게 짓는게 맞나 의구심이 들었는데 bindRetryEvent와 구성이 비슷하니 메서드 명도 비슷하게 지으면 되는거군요! bindCloseEvent로 수정하였습니다.
src/view/ResultView.js
Outdated
|
|
||
| printWinningCount(winningResult) { | ||
| $$winningCounts.forEach((winningCount, index) => { | ||
| this.print(winningCount, this.createRankList(winningResult)[index]); |
There was a problem hiding this comment.
this.createRankList(winningResult)를 그대로 사용하면 forEach문이 실행될 때마다 this.createRankList(winningResult)가 계속 실행될 것 같아요.
const로 한 번 정의해서 사용하는 건 어떠신가요?
There was a problem hiding this comment.
그러네요! print하는 용으로 쓰는건데 안에서 사용하면 forEach가 돌아갈 때마다 계속 실행되네요! 밖으로 빼서 수정하였습니다.
|
리뷰 감사합니다 체프! 아, querySelector 상수화를 하면 그런 단점이 있군요! DOM 요소는 추가나 삭제할 일이 더 많은데 그럴 때마다 상수도 다 삭제하거나 수정해야 할 일이 많아지게 되네요! 모두 상수화 해서 쓰면 좋겠지라고 단순하게 생각했는데 장점과 단점을 생각해가면서 코드 짜는 습관을 들여야겠습니다! 생각해보니까 controller는 domain과 view를 총괄하는 역할이었네요! domain을 수정을 최소화하자에 집중하다보니까 순간적으로 헷갈렸던 거 같아요. (위에서는 view를 써 놓고서…) 또, controller의 retryProcess는 필드를 초기화 하는건데 view를 초기화하는 것을 함께 적는게 맞나 생각이 들었어요. 그런데 controller의 원래 역할을 생각해보면 그냥 두어도 괜찮을 거 같네요! 덕분에 궁금한 점에 대해서 모두 해결이 되었습니다! |
There was a problem hiding this comment.
빠르게 피드백 반영해주셔서 감사합니다! 머지하겠습니다 :) 이번 미션도 수고 많으셨어요.
미션과는 별도로 개인적으로 궁금한 점이 있어서 한 가지 질문만 드리고자 합니다.
이번 미션에서 BEM을 적용해주셨는데, 개인적인 소감이 어떠신지 궁금해요. BEM을 사용하면서 어떠셨나요?
사설을 풀어놓자면, 저는 CSS 방법론을 적용하는 것을 그렇게 좋아하지 않아요. 특히 BEM은 컨벤션에 맞추어서 네이밍을 하기에는 너무 제약이 많다고 생각하거든요. 그래서 네이밍하기도 어렵고, 복잡한 네이밍 방법을 구성원 모두가 학습해야 한다는 점에서 아쉬운 점이 많다고 생각해요. 그 때 그 때 팀에 따라서 적절한 네이밍과 컨벤션을 부여한다면 그것으로 충분하다고 생각하고 있어요.
뭔가 이번 미션에서 해온이 BEM을 적용해주셔서 문득 해온의 생각은 어떠셨는지 궁금해져서 질문드려 봅니다.
|
사실 전 처음 배울 때부터 BEM으로 작성해왔어요. 그래서 아무래도 BEM이 편하기 때문에 주로 사용하고 있는데요. 규칙이 없으면 너무 중구난방이고, 알아보기 힘들더라구요. 특히 팀 프로젝트를 하면 서로 다른 팀원의 class명에 더 혼돈이 오기도 하구요...! BEM은 아무래도 HTML 길이가 길어지고 중첩이 많아지고 이러면 BEM 네이밍 방식에 끼워넣기 쉽지 않더라구요! |
|
설명해주셔서 감사드립니다 :) |
안녕하세요 체프! 2단계 완료해서 올립니다~~!
로또 웹 페이지
구조
궁금한 점
querySelector를 이용해 요소를 선택한 부분을 Dom.js 파일에 넣었는데 ( ex) export const$budgetForm = $ ('.budget__form')) 이 부분을 Element.js 파일을 따로 만들어서 분리하는 것이 더 좋을까요?? Dom.js에 적기에는 파일이 통일성 없어보이고 Element.js에 적기에는 너무 다 분리해놓은 느낌이라 고민됩니다...!
innerText와 textContent 중 어느것을 더 선호하시나요?? 둘의 차이점을 찾아봤는데 반환할 때의 차이점만 있더라구요. 저는 반환용으로 쓰는게 아닌데 어떤 걸 쓰는게 더 좋을지 궁금합니다!
LottoDomSimulator의 retryProcess 메서드에서 4~7번째 줄은 다 화면에 대한 초기화입니다. LottoDomSimulator에서 해야될 것은 아닌거 같은데 retry button event가 있는 ResultView의 retryHandler 메서드 에서 하려니 거기에서 $totalBudget이랑 $ticketsList에 대한 초기화를 해줘도 되는지 모르겠습니다. (지금 찾아보니까 retryHandler 메서드에 $winningContainer를 가져와 쓰긴 했네요...) 이렇게 섞어쓰는게 맞는건지 의구심이 들어서 여쭈어봅니다...!