-
Notifications
You must be signed in to change notification settings - Fork 21
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 #59
The head ref may contain hidden characters: "Basic-\uAE40\uC218\uC601-week4"
[김수영] Sprint4 #59
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.
수영님 4번째 스프린트 미션 고생하셨습니다.
이전 리뷰 반영하신것도, commit 쪼개신것도 좋습니다.
아래는 전반적인 리뷰사항입니다.
- 동일한 사항에 대해서는 한번만 코멘트를 답니다. 비슷한 경우를 찾아서 변경해보세요~
- 변수명, 함수명에 대해 조금더 고민해보시면 좋겠습니다. button이 아니라 어떤 버튼인지, is로 시작하는 변수는 boolean값으로 한다던지, 지엽적인 변수는 사용하는 위치에 선언하신다던지 가독성을 위해 조금더 고민해보시면 더 좋을 것 같습니다.
- auth.js파일 하나로 login, signup에서의 validation 로직을 같이 처리하셨는데, 공통되는 함수들은 따로 분리하고, login, signUp 로직을 분리하셔도 좋을 것 같습니다.
@@ -64,6 +75,6 @@ | |||
</div> | |||
</div> | |||
|
|||
|
|||
<script src="script/auth.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.
P2:
HTML 문서 다운로드 후 JS가 다운로드 되게 하단에 배치하신 것 같은데
하단에 배치된 스크립트는 구문분석 순서상 추후에 다운로드 되게 만들어주지만 일반적으로 스크립트 태그가 위치되는 head 요소가 아니므로 가독성 측면에서 단점이 있고 이러한 동작은 스크립트 자체 속성을 통해 이끌어 낼 수 있으므로 script async, defer 속성 사용을 추천드립니다.
https://developer.mozilla.org/ko/docs/Web/HTML/Element/script#defer
https://ko.javascript.info/script-async-defer
// 처음에 비활성화 되도록 | ||
submitButtonInvalidate(); |
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.
P3:
처음에 로그인, 회원가입 버튼이 비활성화되어 있길 원하시면
button에 disabled속성을 true로 해주시는게 더 좋을 것 같습니다.
자바스크립트로 처리하는 것보다 명시적이라 동작 파악에도 더 좋습니다.
if (isPasswordVisible === "text") { | ||
passwordInput.type = "password"; | ||
visibilityIcon.src = "../img/btn_visibility_off.png"; | ||
} else { | ||
passwordInput.type = "text"; | ||
visibilityIcon.src = "../img/btn_visibility_on.png"; | ||
} |
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.
P3:
elary return 문을 사용하셔도 좋겠습니다.
if (isPasswordVisible === "text") { | |
passwordInput.type = "password"; | |
visibilityIcon.src = "../img/btn_visibility_off.png"; | |
} else { | |
passwordInput.type = "text"; | |
visibilityIcon.src = "../img/btn_visibility_on.png"; | |
} | |
if (isPasswordVisible === "text") { | |
passwordInput.type = "password"; | |
visibilityIcon.src = "../img/btn_visibility_off.png"; | |
return; | |
} | |
passwordInput.type = "text"; | |
visibilityIcon.src = "../img/btn_visibility_on.png"; | |
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.
이렇게도 작성 가능합니다.
passwordInput.type = isPasswordVisible ? "password" : "text";
visibilityIcon.src = isPasswordVisible? "../img/btn_visibility_off.png" : "../img/btn_visibility_on.png";
const button = event.currentTarget; | ||
const passwordInput = button.previousElementSibling; | ||
const visibilityIcon = button; | ||
const isPasswordVisible = passwordInput.type; |
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.
P1:
is로 시작하는 변수는 boolean값으로 하시는것이 좋습니다.
const isPasswordVisible = passwordInput.type; | |
const isPasswordVisible = passwordInput.type === 'text'; |
@@ -0,0 +1,165 @@ | |||
const loginForm = document.getElementById('login-form'); | |||
const signupForm = document.getElementById('signup-form'); |
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.
P1:
해당 변수의 경우 submitButtonInvalidate 함수에서만 사용하므로 해당 함수 내부에서 선언해주세요.
전역으로 변수를 선언해서 사용하면 해당 변수의 범위가 너무 넓고, 나중에 코드 파악하기가 어려워집니다.
if (passwordInput) { | ||
passwordInput.addEventListener("focusout", checkPassword); | ||
} |
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.
P1:
이벤트를 연결하는 코드들이 함수 선언문 사이사이에 들어가 있어서 코드 파악이 어렵습니다.
가능하면 변수 선언 => 함수 선언 => 이벤트 바인딩문
과 같은 순서처럼 관련있는 코드끼리 기준을 정해서 묶어 작성하시는 것을 추천드립니다.
function submitButtonInvalidate() { | ||
if (signupForm) { | ||
if (isEmailValidate && isPasswordValidate && isPasswordConfirmValidate && isNameValidate) { | ||
signupButton.disabled = false; | ||
window.location.href="/login.html"; | ||
} else { | ||
signupButton.disabled = true; | ||
} | ||
} else if (loginForm) { | ||
if (isEmailValidate && isPasswordValidate) { | ||
loginButton.disabled = false; | ||
window.location.href="/items.html"; | ||
} else { | ||
loginButton.disabled = true; | ||
} | ||
} | ||
} |
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.
P2:
중첩 if 문은 코드파악하기 어렵게 만듭니다.
가능하면 아래와 같이 너무 중첩되지 않게 작성해주세요.
function submitButtonInvalidate() { | |
if (signupForm) { | |
if (isEmailValidate && isPasswordValidate && isPasswordConfirmValidate && isNameValidate) { | |
signupButton.disabled = false; | |
window.location.href="/login.html"; | |
} else { | |
signupButton.disabled = true; | |
} | |
} else if (loginForm) { | |
if (isEmailValidate && isPasswordValidate) { | |
loginButton.disabled = false; | |
window.location.href="/items.html"; | |
} else { | |
loginButton.disabled = true; | |
} | |
} | |
} | |
function initSignupForm() { | |
const isValid = isEmailValidate && isPasswordValidate && isPasswordConfirmValidate && isNameValidate; | |
// 이코드가 잘 이해가 안됩니다. 버튼의 disabled설정이 바뀌는건 input 값이 변경될때마다 check해서 값이 valid하면 disabled를 변경해줘야하고, 페이지 이동을 버튼 click 이벤트가 발생했을때 이동되야하는거 아닌가 싶네요. | |
if (isValid) { | |
signupButton.disabled = false; | |
window.location.href="/login.html"; | |
return; | |
} | |
signupButton.disabled = true; | |
} | |
function initLoginForm() { | |
const isValid = isEmailValidate && isPasswordValidate | |
if (isValid) { | |
loginButton.disabled = false; | |
window.location.href="/items.html"; | |
return; | |
} | |
loginButton.disabled = true; | |
} | |
// 조건 모두 충족 시 로그인, 회원가입 버튼 활성화 | |
function submitButtonInvalidate() { | |
const loginForm = document.getElementById("login-form"); | |
const signupForm = document.getElementById("signup-form"); | |
if (signupForm) initSignupForm() | |
if (loginForm) initLoginForm() | |
} |
요구사항
기본
로그인
회원가입
심화
배포 주소
https://glistening-bavarois-79cc7f.netlify.app/login
https://glistening-bavarois-79cc7f.netlify.app/signup