-
Notifications
You must be signed in to change notification settings - Fork 77
[장준혁] Sprint4 #237
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
[장준혁] Sprint4 #237
The head ref may contain hidden characters: "part1-\uC7A5\uC900\uD601-sprint4"
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.
안녕하세요 준혁님, 미션 진행하느라 수고하셨습니다 👍
기능 구현을 잘 하셨고 코드 흐름도 좋습니다만, 평소 준혁님 코드에서 느껴지는 '깔끔함'이 덜했습니다.
부랴부랴 기능구현에만 초점을 맞춘 느낌이랄까요? 전반적으로 리팩토링 부탁드립니다!
엘리먼트를 반환하는 querySelector와 getElementById가 속도 차이가 있다고 들었는데 현업에서 getElementById로 반영할 만큼 차이가 유의미한지 궁금합니다.
=> 아무래도 getElementById는 찾으려는 대상이 조금더 specific하고 querySelector 는 general해서 전자가 속도가 더 빠를것으로 예상되나, 웹사이트 퍼포먼스에 영향을 줄만큼의 차이는 아니라고 생각합니다. 크게 구분없이 편하신걸로 사용해주세요 :D
scripts/auth/val-signin-text.js
Outdated
| @@ -0,0 +1,82 @@ | |||
| let emailVal = false; | |||
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.
[참고해주세요]
전역으로 관리하는것 보다는 element안에 있는 값을 재활용 하시는걸 추천드립니다.
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.
즉시 실행 함수로 바꾸어 전역 함수를 함수 내부 변수로 바꾸어도 괜찮을까요?
scripts/auth/val-signin-text.js
Outdated
| const submitBtn = document.getElementById("submitBtn"); | ||
|
|
||
| if (emailVal && pwdVal) { | ||
| submitBtn.style.backgroundColor = "var(--btn-blue1)"; |
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.
[수정해주세요]
style을 직접 조작하는것 보다는 disabled 속성을 활용해주세요.
scripts/auth/val-signin-text.js
Outdated
|
|
||
| email.addEventListener("focusout", () => { | ||
| if (email.value === "") { | ||
| email.style.outline = "2px solid var(--error-red)"; |
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.
[수정해주세요]
여기도 style을 직접 조작하기 보다는 class를 땟다 붙였다 해주세요. 휴먼에러가 나올수 있습니다.
|
…ithub-actions [Fix] delete merged branch github action
체크리스트 [기본]
로그인
회원가입
체크리스트 [심화]
주요 변경사항
스크린샷
스프린트 미션 4
멘토에게