-
Notifications
You must be signed in to change notification settings - Fork 42
[문지영] sprint4 #137
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
The head ref may contain hidden characters: "Basic-\uBB38\uC9C0\uC601-sprint4"
[문지영] sprint4 #137
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.
수고하셨습니다!
주요 리뷰 포인트
- 유지보수를 고려한 개발
function showError(inputEl, message) { | ||
const errorEl = inputEl.parentElement.querySelector('.error-message'); | ||
inputEl.classList.add('error'); | ||
if (errorEl) errorEl.textContent = message; | ||
} | ||
|
||
function clearError(inputEl) { | ||
const errorEl = inputEl.parentElement.querySelector('.error-message'); | ||
inputEl.classList.remove('error'); | ||
if (errorEl) errorEl.textContent = ''; | ||
} | ||
|
||
// 이메일 input에서 focus out 할 때, | ||
// 값이 없을 경우 input에 빨강색 테두리와 아래에 “이메일을 입력해주세요.” 빨강색 에러 메세지를 보입니다. | ||
// 이메일 input에서 focus out 할 때, | ||
// 이메일 형식에 맞지 않는 경우 input에 빨강색 테두리와 아래에 “잘못된 이메일 형식입니다” 빨강색 에러 메세지를 보입니다. | ||
|
||
const emailInput = document.querySelector('#login-email'); | ||
const emailPattern = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/; | ||
|
||
emailInput.addEventListener('focusout', function() { | ||
if (emailInput.value === '') { | ||
showError(emailInput, '이메일을 입력해주세요.'); | ||
} else if (!emailPattern.test(emailInput.value)) { | ||
showError(emailInput, '잘못된 이메일 형식입니다'); | ||
} else { | ||
clearError(emailInput); | ||
} | ||
}); | ||
|
||
// 비밀번호 input에서 focus out 할 때, | ||
// 값이 없을 경우 아래에 “비밀번호를 입력해주세요.” 에러 메세지를 보입니다 | ||
// 비밀번호 input에서 focus out 할 때, | ||
// 값이 8자 미만일 경우 아래에 “비밀번호를 8자 이상 입력해주세요.” 에러 메세지를 보입니다. | ||
|
||
const pwInput = document.querySelector('#password'); | ||
|
||
function validatePassword() { | ||
if (pwInput.value === '') { | ||
showError(pwInput, '비밀번호를 입력해주세요.'); | ||
} else if (pwInput.value.length < 8) { | ||
showError(pwInput, '비밀번호를 8자 이상 입력해주세요.'); | ||
} else { | ||
clearError(pwInput)}; | ||
} | ||
|
||
pwInput.addEventListener('focusout', validatePassword) | ||
pwInput.addEventListener('input', validatePassword) |
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.
예시)
// 유효성 검사 함수들
const validators = {
required: (value) => ({
isValid: value !== '',
errorMessage: '이메일을 입력해주세요.'
}),
email: (value) => ({
isValid: /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/.test(value),
errorMessage: '잘못된 이메일 형식입니다'
}),
password: (value) => ({
isValid: value.length >= 8,
errorMessage: '비밀번호를 8자 이상 입력해주세요.'
})
};
// 에러 표시 함수
function toggleError(inputEl, message = '') {
const errorEl = inputEl.parentElement.querySelector('.error-message');
inputEl.classList.toggle('error', message !== '');
if (errorEl) errorEl.textContent = message;
}
// 유효성 검사 실행 & 에러 토글
function validateInput(inputEl, validationRules) {
const value = inputEl.value;
for (const rule of validationRules) {
const { isValid, errorMessage } = validators[rule](value);
if (!isValid) {
toggleError(inputEl, errorMessage);
return false;
}
}
toggleError(inputEl);
return true;
}
// 이메일 유효성 검사
const emailInput = document.querySelector('#login-email');
emailInput.addEventListener('focusout', () => {
validateInput(emailInput, ['required', 'email']);
});
// 비밀번호 유효성 검사
const pwInput = document.querySelector('#password');
const validatePassword = () => {
validateInput(pwInput, ['required', 'password']);
};
pwInput.addEventListener('focusout', validatePassword);
pwInput.addEventListener('input', validatePassword);
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.
이렇게 나눠놓고보니, validators와 toggleError, validateInput은 login뿐만이 아닌 정해둔 유효성 검사 규칙을 기준으로 검사를 수행하고 에러를 토글해야하는 다른 페이지 (e.g #) 에서도 필요해보여요. 이럴땐 모듈화하고 재사용해주면 코드 중복도 줄어들수있겠죠? :)
const allInput = document.querySelectorAll('input'); | ||
const loginBtn = document.querySelector('#login-btn'); | ||
|
||
function checkAllValid() { | ||
let isValid = true; | ||
|
||
allInput.forEach((input) => { | ||
const errorText = input.parentElement.querySelector('.error-message').textContent; | ||
if (input.value ==='' || errorText !== '') { | ||
isValid = false; | ||
} | ||
}); | ||
|
||
loginBtn.disabled = !isValid; | ||
} | ||
|
||
allInput.forEach((input) => { | ||
input.addEventListener('input', checkAllValid); | ||
input.addEventListener('focusout', checkAllValid); | ||
}); | ||
|
||
// 활성화된 ‘로그인’ 버튼을 누르면 “/items” 로 이동합니다. | ||
|
||
loginBtn.addEventListener('click', () => { | ||
if (!loginBtn.disabled) { | ||
window.location.href = '/itmes'; | ||
} | ||
}); |
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.
NIT: 이런 코드들은 UI에 결합되어있어 쉽게 바뀔 수 있고 (변경의 가능성이 큼) 재사용하기 쉽지 않아요. 위에 드린 코멘트에 있는 함수들은 재사용하기 수월하고요. 따라서 이런 코드는 모듈화의 대상이 된다기보다는 지금처럼 login.js (페이지별 js파일)에 있는게 적절하겠죠? :)
@@ -66,7 +64,7 @@ function validatePassword() { | |||
} | |||
|
|||
function validatePasswordMatch() { | |||
if (pwcheckInput.value !== pwInput.value && pwcheckInput !== '') { | |||
if (pwcheckInput.value === '' || pwcheckInput.value !== pwInput.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.
pwcheckInput.value === ''
일때는 '비밀번호를 입력해주세요' 와 같은 에러메시지를 따로 보여주는게 좋지않을까요? 논리합 연산자를 사용하게되면 두가지 조건중 하나만 true여도 if문 블락이 실행되니까, 사용자 입장에서 잘못된 피드백을 받게 될 수 있겠죠?
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.
이 파일에서도 모듈화된 함수를 사용해주면 코드 중복을 줄일 수 있겠죠? :)
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
멘토에게