-
Notifications
You must be signed in to change notification settings - Fork 15
[이재익_FrontEnd]3주차 과제 제출합니다. #32
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
base: main
Are you sure you want to change the base?
Conversation
|
각 주차마다 새롭게 파일을 만들고 계신데 기존의 코드를 수정해서 이어가시는게 좋을 것 같아요. 이번 리뷰는 이번 주차 커밋 기준으로 진행할게요 |
ff1451
left a comment
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.
|
|
||
|
|
||
| </div> | ||
| <script src="lotto.js"></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.
현재 lotto.js라는 파일이 존재하지 않아서 JS 기능이 작동하지 않았을 것 같아요. 파일명을 수정하시거나 경로를 수정하시면 됩니다
| } | ||
|
|
||
| .title{ | ||
| font-family: Noto Sans KR; |
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.
폰트 설정이 필요한 모든 부분에 개별적으로 적용중인데 상위 부모에서 지정후 상속하는 방식으로 하면 중복을 줄일 수 있어요
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>로또 번호 생성기기</title> | ||
| <link href="lotto.css" rel="stylesheet"> |
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.
css도 js와 동일하게 경로 변경이 필요해 보여요
| .result { | ||
|
|
||
|
|
||
| width: 470px; | ||
| height: 40px; | ||
|
|
||
|
|
||
| } | ||
| .ballsetting { | ||
| display: flex; |
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.
코드 전반적으로 개행, 들여쓰기가 불규칙적이에요. 통일하는 편이 다른 사람과의 협업 시 가독성, 효율이 좋으니 수정하시면 좋을 것 같아요
|
|
||
| } | ||
|
|
||
| .mynumbertitlecss { |
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.
변수나 클래스명 등을 선언할 때 pascal, camel, kebab 등 여러 네이밍 컨벤션이 존재해요
html/css에서는 kebab case를 주로 사용하니 확인해보시면 좋을 것 같아요
| @@ -0,0 +1,118 @@ | |||
| let creatNumberButton1 = document.querySelector(".ball.ball1"); | |||
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.
모든 변수 선언을 let으로 선언했는데 let, const, var는 어떤 차이가 있을까요?
| let pushButton = document.querySelector(".button"); | ||
| let pushButton1 = document.querySelector(".button1"); |
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.
두 버튼의 목적, 기능이 다르다보니 이를 표현해줄 수 있도록 변수명과 클래스 명을 선언하시는게 좋아요
저라면 pushButton, pushButton1 보단 purchaseLottoButton, generateLottoNumberButton 처럼 했을 것 같아요
| let tempNumber = []; | ||
| while (tempNumber.length < 6) { | ||
| let n = randomNumber(); | ||
| if (!tempNumber.includes(n)) { | ||
| tempNumber.push(n); | ||
| } | ||
| } |
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.
이와 동일한 동작을 하는 로직이 중복되고 있어요 재사용할 수 있도록 구성을 한다면 코드 중복을 줄일 수 있어요
| } | ||
| let ball = document.querySelector(`.ball.ball${i}`); | ||
| ball.textContent = num; | ||
| winningNumber.push(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.
중복되지 않는 숫자만 배열에 추가하고자 한다면 해당 부분은 불필요한 코드라고 생각돼요

No description provided.