Conversation
| @@ -0,0 +1,74 @@ | |||
| const inputEmail = document.getElementById("input-email"); | |||
| const errorMsgEmail = document.querySelector(".error-message.email"); | |||
There was a problem hiding this comment.
p3;
맥락상 inputEmail, errorMsgEmail 변수명 보다는
emailInput, emailErrorMessage 와 같은 표현이 조금 더 적절할 것 같아요!
| const errorMsgEmail = document.querySelector(".error-message.email"); | ||
| let flagEmail = 0; | ||
| inputEmail.addEventListener("focusout", ({ target }) => { | ||
| const pattern = /^[A-Za-z0-9_\.\-]+@[A-Za-z0-9\-]+\.[A-za-z0-9\-]+/; |
There was a problem hiding this comment.
p4;
요런 이메일 정규식 패턴 같은 경우에는 일반적으로 불변하고, 여러곳에서 재사용하기 때문에
특정 함수 스코프 내에 가두기 보다는 따로 외부에서 상수로 정의하여 가져다 사용하면 더 좋을 것 같아요!
const VALID_EMAIL_PATTERN = /^[A-Za-z0-9_\.\-]+@[A-Za-z0-9\-]+\.[A-za-z0-9\-]+/;| const inputEmail = document.getElementById("input-email"); | ||
| const errorMsgEmail = document.querySelector(".error-message.email"); | ||
| let flagEmail = 0; | ||
| inputEmail.addEventListener("focusout", ({ target }) => { |
There was a problem hiding this comment.
객체 구조 분해할당을 사용하셔서 target을 잘 가져오셨네요👍
| flagEmail = 1; | ||
| } | ||
| checkInputs(); | ||
| }); |
There was a problem hiding this comment.
p3;
focusout 되는 시점에 유효성을 검사하고, 그에 따른 로그인 버튼 활성화를 잘 구현하신 것 같아요!
하지만, 아래 사항들을 고려하여, 조금 더 가독성과 유지보수성을 향상시킬 수 있을 것 같아요.
-
addEventListener에 전달되는 함수는 별도로 분리해서 작성
-> 기능상 차이는 없지만, 어떤 이벤트에 어떤 함수가 동작하는지 1차적으로 함수명으로 유추하기 용이할 것 같아요. -
공통된 부분들을 별도의 함수로 정의
-> 중복되는 부분들이 보이는 것 같아요. 예를들어 아래와 같은 부분이요!
target.classList.add("error");
errorMsgEmail.style.display = "block";요런 부분들은 어떻게 하면 재사용할 수 있을지 고민을 해보고, 별도의 함수로 쪼개는 연습을 하면 좋을 것 같아요!
-
변수명을 더 직관적으로 작성하기
-> 예를들어flagEmail이라는 변수명 보다는isEmailValid와 같은 직관적인 변수명이 더 읽기 좋을 것 같아요! -
추가로 flag에 0, 1 값을 할당하는 대신 명확하게 불리언으로 표시하는게 좋을 것 같아요!
아래는 재구성한 예시입니다. 보시고, 이런 느낌으로도 작성할 수 있구나를 생각하시면서 password 관련한 함수도 한 번 개선해보시면 좋을 것 같아요!
// 이메일 값 검증
const validateEmail = (emailValue = "") => {
const validEmailPattern = /^[A-Za-z0-9_\.\-]+@[A-Za-z0-9\-]+\.[A-za-z0-9\-]+/;
if (emailValue === '') return {isValid: false, errorMessage: '이메일을 입력해주세요.'};
if (!validEmailPattern.test(emailValue)) return {isValid: false, errorMessage: '잘못된 이메일 형식입니다.'};
return {isValid: true, errorMessage: null};
}
// 보통 이벤트리스너 핸들러에 아래와 같이 handle을 붙이곤 합니다!
const handleFocusOutEmailInput = ({target}) => {
const emailValue = target.value;
const { isValid, errorMessage } = validateEmail(emailValue);
isEmailValid = isValid;
if (!isValid) {
target.classList.add("error");
errorMsgEmail.style.display = "block";
errorMsgEmail.textContent = errorMessage;
disableLoginButton(); --> 여기서는 이메일이 유효하지 않으니, checkInputs와 같이 모든 인풋 항목을 검사하지 않고 곧바로 로그인 버튼을 비활성화 할 수 있을 것 같아요!
return;
}
target.classList.remove("error");
errorMsgEmail.style.display = "none";
handleLoginButtonActivate();
}
inputEmail.addEventListener("focusout", handleFocusOutEmailInput)
// 로그인 버튼 비활성화
const disableLoginButton = () => {
loginBtn.disabled = true;
loginBtn.classList.remove("enabled");
}
// 로그인 버튼 활성화
const activateLoginButton = () => {
loginBtn.disabled = false;
loginBtn.classList.add("enabled");
loginBtn.addEventListener("click", () => {
window.location.href = "../items/index.html";
});
}
// 기존 checkInputs()
const handleLoginButtonActivate = () => {
if (isEmailValid && isPasswordValid) {
activateLoginButton();
return;
}
disableLoginButton();
}| if (flagEmail && flagPassword) { | ||
| loginBtn.disabled = false; | ||
| loginBtn.classList.add("enabled"); | ||
| loginBtn.addEventListener("click", () => { |
There was a problem hiding this comment.
p4;
개인적인 의견으로는 애초에 로그인 버튼 자체에 disabled가 걸리면, 동작을 하지 않으니
로그인 버튼 자체에 기본적으로 클릭시 window.location.href = "../items/index.html"; 할 수 있도록 정의 해놓고
여기서 동적으로 addEventListener하는 부분을 제거해도 괜찮을 것 같아요!
| "../asset/icon/btn_visibility_off.png"; | ||
| flagVisibility = 0; | ||
| } | ||
| }); |
There was a problem hiding this comment.
p3;
요 부분도 기능을 잘 구현하셨는데, 삼항 연산자와 변수 통합을 잘 활용하시면
조건문 없이도 더 깔끔한 코드로 개선이 가능할 것 같아요!
이 부분도 마찬가지로 리스너에 함수 본문을 다 작성하시는 대신, 별도의 함수로 선언한뒤 가져다가 사용하시는게 더 깔끔할 것 같아요!
let isPasswordVisible = false; --> 요렇게 변수를 불리언으로 개선하면 주석에 표시하지 않고, 보기만 해도 명확할 것 같아요!
const handleClickTogglePasswordVisibilityButton = () => {
const inputIcon = document.querySelector(".password-show-icon");
const inputType = isPasswordVisible ? 'password' : 'text';
const iconType = isPasswordVisible ? 'off' : 'on';
passwordInput.type = inputType;
inputIcon.src = `../asset/icon/btn_visibility_${iconType}.png`
isPasswordVisible = !isPasswordVisible; --> visible 버튼은 무조건 현재 상태의 반대만 될 것 같아요!
}
pwVisibilityIcon.addEventListener('click', handleClickTogglePasswordVisibilityButton)| @@ -0,0 +1,126 @@ | |||
| const inputEmail = document.getElementById("input-email"); | |||
| const errorMsgEmail = document.querySelector(".error-message.email"); | |||
| let flagEmail = 0; | |||
There was a problem hiding this comment.
p3;
사실상 login에 있던 로직들과 signup에 있는 로직은 대부분 모두, 재사용이 가능할 것 같아요!
다만, �요렇게 flagEmail과 같이 해당 스크립트 내에서 초기화되는 변수들 때문에, 현재 재사용이 어려우실 것 같아요.
이런 이유들 때문에, 사실 저렇게 스크립트 내에서 전역변수로 선언하는 건 부작용이 많아요!
따라서 이렇게 flag 등의 전역변수로 접근하는 것보다는
해당 인풋들 요소 내에 적절한 어트리뷰트를 설정하여 해당 어트리뷰트를 조작하거나,
혹은 is-error is-valid 와 같은 클래스명을 통해 인풋들의 유효성 여부를 판단하는게 더 좋은 방법일 것 같아요!
ex)
기본 마크업
<input id="input-email" data-valid="false">
// 유효한 경우 어트리뷰트 변경
�email.setAttribute("data-valid", "true");
|
고생 많으셨어요, 정민님! 공통되는 부분들을 따로 모듈화하기 위해서는 그러면 분명히 공통적으로, 사용하기 애매한 부분들이 보일거에요. 사실 정답은 없습니다! 앞으로 계속 여러 변수를 선언하고, 그것들을 함수로 재구성하는 과정을 반복하다 보면 이외에 코드를 보며 몇가지 개선하면 좋을 점에 대해서 간단한 리뷰들 남겼습니다! +) 리뷰를 남긴 코멘트 위의 p(1~5); 마크의 의미는 아래와 같습니다! 참고하면 좋을 것 같아요! |
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
로그인 화면
회원가입 화면
멘토에게