-
Notifications
You must be signed in to change notification settings - Fork 42
[김동희] Sprint4 #147
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-\uAE40\uB3D9\uD76C-sprint4"
[김동희] Sprint4 #147
Conversation
1. Email validation 추가 정규식 활용 2. Password validation 추가 3. 모든 확인 조건이 만족되었을 때, login/# 버튼 활성화 기능 추가 4. Password 표시를 위한 Toggle Btn 추가
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 | ||
const verifyInputs = () => { | ||
authFormUtils.verify(verifiationList) |
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.
authFormUtils.verify(verifiationList) | |
const isValid = authFormUtils.verify(verifiationList) |
사소하지만, 이런식으로 변수로 만들어주면 함수 내부를 들여다보지않아도 실행 결과값에 대한 예측이 좀 더 쉬워지겠죠? :)
* 뒤로 가기 등 페이지를 복구했을 때, 브라우저가 입력값을 복원하는 과정에서 요구사항의 focusout 이벤트가 발생하지 않음. | ||
* 따라서, pageshow 이벤트 발생 후 email 값이 있는 경우 (입력값이 복원된 경우) 강제로 focusout 이벤트 실행 | ||
*/ | ||
window.addEventListener("pageshow", () => { |
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.
코드들이 일련의 흐름으로 실행된다기보다는 흩어져있어 파악이 어려워보여요.
이벤트리스너의 콜백에 직접적으로 관련 로직을 작성하기보다는 개별 함수로 작업을 분리해주고, 이런식으로 initialize를 담당하는 함수를 하나 만들어보면 어떨까요?
// 페이지 복원 시 폼 상태 검증
const validateFormOnRestore = () => {
const inputs = [
{ element: inputEmail, detail: detailEmail, field: 'email' },
{ element: inputPw, detail: detailPw, field: 'password' }
];
inputs.forEach(({ element, detail, field }) => {
if (element.value) {
validateField(element, detail, field);
}
});
};
// 이벤트 리스너 설정
const setupEventListeners = () => {
// 페이지 복원 이벤트
window.addEventListener('pageshow', validateFormOnRestore);
// 이메일 입력 필드
inputEmail.addEventListener('focusout', (e) => {
validateField(e.target, detailEmail, 'email');
});
// 비밀번호 입력 필드
inputPw.addEventListener('focusout', (e) => {
validateField(e.target, detailPw, 'password');
});
// 비밀번호 표시 토글
togglePw.addEventListener('click', togglePasswordVisibility);
};
// 초기화
setupEventListeners();
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 | ||
const verifyInputs = () => { |
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.
해당 함수가 하는 역할이 input을 유효성검사하기보다는, 유효성 검사 결과를 button UI state에 반영하는일에 가깝지않을까요?
const isCorrect = authFormUtils.isCorrectEmail(e.target.value); | ||
|
||
if (!e.target.value) { | ||
authFormUtils.changeDetail(e, detailEmail, "이메일을 입력해주세요"); | ||
verifiationList.email = false; | ||
} else if (!isCorrect) { | ||
authFormUtils.changeDetail(e, detailEmail, "잘못된 이메일 형식입니다."); | ||
verifiationList.email = false; | ||
} else { | ||
authFormUtils.changeDetail(e, detailEmail); | ||
verifiationList.email = 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.
유효성 검사 규칙에 따른 유효성 검사 결과를 반환하는 형태의 함수가 모듈화되어있지않아 페이지 단위의 js에서 매번 유효성 검사를 수행하기위한 코드 블락을 작성해야하는게 비효율적으로 보여요.
이런식으로 간소화해보면 어떨까요?
예시)
// 유효성 검사 함수들
export const validators = {
email: (value) => {
if (!value) return { isValid: false, message: '이메일을 입력해주세요' };
if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value)) {
return { isValid: false, message: '올바른 이메일 형식이 아닙니다' };
}
return { isValid: true, message: '' };
},
password: (value) => {
if (!value) return { isValid: false, message: '비밀번호를 입력해주세요' };
if (value.length < 8) {
return { isValid: false, message: '비밀번호는 8자 이상이어야 합니다' };
}
return { isValid: true, message: '' };
}
};
// 입력 필드 검증 함수
const validateField = (inputEl, detailEl, field) => {
const value = inputEl.value.trim();
const result = validators[field](value);
if (result.isValid) {
authFormUtils.changeDetail(inputEl, detailEl);
} else {
authFormUtils.changeDetail(inputEl, detailEl, result.message);
}
updateFormState(field, value, result.isValid);
};
return MIN_PW_LEN <= pw.length; | ||
}; | ||
|
||
export const changeDetail = (event, detailElement, text = "") => { |
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.
함수 이름이 좀 모호하게 느껴져요. 결국 이 함수가 하는 일이 유효성 검사 상태에 따른 UI state 업데이트라면 updateFieldState 정도는 어떨까요?
질문에 대한 답변
그랬군요! 디테일을 볼줄 아시네요 👍 둘중에 더 편한 방법으로 시도해보세요! |
요구사항
배포
Here
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
멘토에게
저는 window객체의 pageshow 이벤트 리스너를 통해 페이지 실행 시 강제로 focusout 이벤트를 1회 실행시키도록 구현했는데요, 뭔가 좋은 구조라는 느낌은 안 들어서... 입력값 복원을 유지한 채로(input value를 초기화하지 않고) 더 나은 방법이 있는가에 대해 여쭤봅니다.