-
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
[2단계 - 웹 기반 로또 게임] 가브리엘(윤주현) 미션 제출합니다. #252
Conversation
Co-authored-by: Minjae Kim <smallkdb@gmail.com>
- 입력 검증에 관한 내용 추가
- 로또 미션에서 주어진 제약 사항 반영
Co-authored-by: Minjae Kim <smallkdb@gmail.com>
Co-authored-by: Minjae Kim <smallkdb@gmail.com>
- 구입 금액 - 당첨 번호 - 보너스 번호 - 다시 시작 커맨드
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.
가브리엘 안녕하세요! 이번 단계 미션도 수고 많으셨어요.
Github Pages로 배포하신 링크를 공유해주신 덕분에 편하게 리뷰할 수 있었어요.
각 코드 라인에 코멘트를 달아두었으니 확인 부탁드릴게요!
JS에서 전역 변수는 어떻게 관리해야 하는가?
모듈을 사용하는 경우의 자바스크립트에서 전역 변수를 어떻게 관리해야 하는지 물어보신 것이 맞으실까요?
우선 전역 변수는 데이터의 흐름을 복잡하게 만드는 원인 중 하나이기 때문에 최대한 사용하지 않는 것이 가장 좋다고 생각합니다. 왠지 언급해야 할 것 같아서 말씀드려 봅니다.
저라면 window 객체에 두거나, 전역 상태 관리 라이브러리를 사용할 것 같습니다.
제가 생각하기에는 이번 미션에서는 가브리엘이 코드에서 작성하신 것처럼 Global 클래스를 이용한 스토어를 만드신 건 합당한 방법이었다고 생각합니다.
이벤트 발생을 감지하여 DOM을 그리고 싶을 때에는 어떻게 해야 하는가?
다양한 방법이 있겠지만 저라면 CustomEvent를 활용해볼 것 같습니다. 한 번 찾아보시면 도움이 될 거예요.
.eslintrc.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"rules": { | |||
"max-lines-per-function": ["error", 10], | |||
// "max-lines-per-function": ["error", 10], |
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.
네. 삭제했습니다!
index.html
Outdated
<!-- <script> | ||
var store = {}; | ||
</script> --> |
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.
새로 만든 Store 클래스가 제대로 동작하지 않으면 복구하려고 주석 처리 해놨었는데, 깜빡하고 삭제를 못했습니다. 삭제하겠습니다!
index.html
Outdated
var store = {}; | ||
</script> --> | ||
|
||
</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.
EOL!
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/css/layout.css
Outdated
|
||
.p-2 { | ||
padding: 0.5rem; | ||
} |
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.
Tailwind CSS처럼 스타일링하시는 것을 선호하시나봐요. 어떤 장단점을 느끼셨나요?
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.
figma 시안 문서에서 컴포넌트 간의 간격이 정해져있는 줄 모르고 빠르게 디자인 하기 위해서 생성했던 코드였는데, 더이상 사용하지 않아서 삭제하겠습니다.
저는 원래 css 대신 부트스트랩을 사용했었는데, 위와 같은 스타일링 방식으로 빠르게 디자인을 잡아야 하는 상황에서 장점을 느꼈습니다.
하지만, 세밀한 간격 조절이 어렵다는 단점이 있었던 것 같습니다.
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.
말씀해주신 것처럼 세밀한 간격 조절은 어려울 수 있지만 디자인에서 간격에 대해서도 스타일 정의가 되어있을 때는 오히려 편할 수도 있겠다는 생각이 드네요 :)
@@ -0,0 +1,91 @@ | |||
.modal { | |||
display: none; |
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.
(뜬금없는 질문) display: none
과 visibility: hidden
의 차이점은 무엇일까요?
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.
display: none
은 해당 영역을 완전히 제거하여 사라진 것 처럼 만드는 것으로 알고 있고, visibility: hidden
은 공간과 영역 자체는 존재하지만 눈에 보이지 않게 처리 하는 것으로 알고 있습니다!
@@ -0,0 +1,11 @@ | |||
/* eslint-disable no-dupe-class-members */ | |||
class Global { |
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.
자바스크립트 모듈에서 전역 변수를 관리하고자 Global 클래스를 만드신 걸까요?
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.
네. 맞습니다.
처음에는 indext.html의 <script>태그에서 var 객체변수를 선언해서 window 객체에 연결하고 이를 참조하는 방식으로 전역 변수를 관리했었는데, var를 안쓰는게 좋을 것 같아서 위와 같이 처리했습니다.
src/css/modal.css
Outdated
background-color: rgb(0,0,0); | ||
background-color: rgba(0,0,0,0.4); |
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.
background-color가 중복 정의되어있네요.
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 form = document.getElementById("money-submit"); | ||
form.onsubmit = function (event) { |
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.
(뜬금없는 질문) onsubmit과 addEventListener('submit', () => { /* ... */ })
과의 차이점이 있을까요?
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.
onsubmit과 다르게 addEventListener의 경우에는 하나의 이벤트 대상에 여러 이벤트 리스너를 등록할 수 있다는 장점이 있는 것 같습니다..!
try { | ||
LottoValidator.checkMoney(money); | ||
global.setStore('lottos', generateLottos((money))); | ||
event.target.money.value = ''; |
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.
이벤트 객체를 통해서 DOM을 수정하고 있는 코드가 맞을까요? getElementById와 같이 DOM 선택자로 접근하지 않은 이유가 있으신가요?
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 form = document.getElementById("money-submit");
form.onsubmit = function (event) {
event.preventDefault();
const money = event.target.money.value;
try {
LottoValidator.checkMoney(money);
global.setStore('lottos', generateLottos((money)));
event.target.money.value = '';
renderPurchaseResults();
renderInputWinningNumber();
} catch (error) {
alert(error.message);
}
};
이 동작은 다른 함수가 아닌 자기 자신인 form에서 발생하는 event를 받아서 동작하기에, 해당 event(본인)의 target을 직접 조작할 수 있어서 DOM 선택자로 접근을 하지 않았습니다. (event가 곧 자기 자신과 연관이 되어있다고 생각했습니다.)
getElementById로 접근해서 수정하는게 나을까요?
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.
저는 코드에서 가독성을 중요하게 생각하는 편인데요. 그래서 그런지 "명시"하는 것을 좋아해요.
만약에 event.target을 이용해서 DOM에 접근했다면, event가 어떤 DOM 요소에 걸려있는지 코드를 읽어서 확인해야 하죠.
반면에 getElementById나 querySelector를 통해서 접근한다면, 어떤 요소가 선택되었는지 눈으로 바로 확인할 수 있으니까 더 좋다고 생각해요.
요것은 약간 취향에 따라서 갈릴 수 있을 것 같아요. event.target을 이용해서 DOM에 접근하는 코드를 작성하신 가브리엘의 의견이 어떤지 궁금해서 질문 드려보았어요. 꼭 수정하지 않으셔도 괜찮습니다 :) 답변 감사합니다.
}; | ||
|
||
window.onclick = function (event) { | ||
if (event.target == modal) { |
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.
이유가 있는건 아니고 실수였습니다. 바로 수정하겠습니다..!!
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.
가브리엘! 2단계까지 미션 수행하시느라 수고 많으셨어요.
이만 어프루브하고 머지하겠습니다! 다음 미션도 화이팅입니다!
안녕하세요.
조금 더 빨리 리뷰 요청을 드렸어야 했는데, 이제 서야 하게 됐습니다.
핑계일 수도 있지만, 금주 목요일에 진행했던 테코톡 준비와 졸업식이 겹쳐서 주말이 되어 서야 step2를 완성할 수 있었습니다.
git pages link 입니다.
제가 이번 미션을 진행하면서 계속해서 고민했던 부분에 대해서 정리해봤습니다.
JS에서 전역 변수는 어떻게 관리해야 하는가?
제가 Java를 공부할 때에는, 어떤 프로그램에서 다같이 공유해야 하는 변수나 객체가 있다면 Controller 같은 위치에 static 키워드를 붙여서 어디에서도 동일한 상태의 값을 접근하는 방법을 사용했습니다.
하지만 JS에서는 전역 변수를 사용하기 위해서는 여러 가지 제약이 있다고 생각하는데요,
html에서 스크립트를 직접 작성하는 방식으로 var 변수를 window 객체에 붙여서 사용 하는 방법이 가장 쉽고 간편하겠지만, 지금처럼 모듈 방식의 개발에서는 스코프 제한이 걸려있어서 불편함을 느꼈습니다.
전역 변수나 전역 객체를 손쉽게 관리할 수 있는 방법이 있을까요?
이벤트 발생을 감지하여 DOM을 그리고 싶을 때에는 어떻게 해야 하는가?
React를 사용하다가 바닐라로 개발을 하려다 보니, DOM을 일일이 조작하려는 행위를 어디에서 결정해야 하는지 고민이 많았습니다.
만약에 이 페이지가 변화가 거의 없는 비교적 정적인 기능만 가지고 있다면, 특정 버튼에 이벤트를 거는 것으로 처리했겠지만, 이번에는 특정 행위가 마무리됐을 때, 다음 html 를 그려줘야 해서 다소 어려움이 있었습니다.
step1에서 콘솔 개발을 할 때에는 async/await으로 다음 동작으로 넘어가지 못하게 하는 방식으로 제어를 했다면, 이번에는 이벤트에 이벤트를 물리는 방식으로 개발을 하여 많은 아쉬움이 남았는데, 더 효율적인 방법이 있는지 조언해주시면 감사하겠습니다.