-
Notifications
You must be signed in to change notification settings - Fork 26
[심예진] sprint4 #75
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 #75
The head ref may contain hidden characters: "Basic-\uC2EC\uC608\uC9C4-sprint4"
Conversation
baeggmin
left a comment
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 미션 수행하시느라 고생 많으셨습니다! 👍👍
질문 주신 내용에 대해서는 인라인 코멘트로 남겨두었으니 참고 부탁드립니다.
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 setError(inputEl, errorEl, message) {
errorEl.textContent = message;
inputEl.classList.add("input-error");
}
function clearError(inputEl, errorEl) {
errorEl.textContent = "";
inputEl.classList.remove("input-error");
}
| function checkValidity() { | ||
| const emailValid = validateEmail(emailInput.value); | ||
| const passwordValid = passwordInput.value.length >= 8; | ||
| const passwordDoubleValid = passwordInput.value === passwordDoubleInput.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.
passwordDouble이란 명칭이 조금 모호한 것 같아요. passwordConfirm 어떠신가요?
|
|
||
| //비밀번호 확인 | ||
| passwordInput.addEventListener("blur", () => { | ||
| const value = passwordInput.value.trim(); |
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.
공백 처리를 위한 trim 메소드 사용 좋습니다!👏
| checkValidity(); | ||
| }); | ||
|
|
||
| // //비밀번호 눈 가리기 |
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.
비밀번호 토글 로직을 아주 잘 작성해주셨습니다! 코드만 봤을땐 문제없이 동작할거같은데, 잘 안되었나요??🤔
| if (emailInput.value && passwordInput.value && emailValid && passwordValid && !errors) { | ||
| loginBtn.classList.remove("disabled"); | ||
| } else { | ||
| loginBtn.classList.add("disabled"); |
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.
현재는.disabled 클래스만 변경하고 있는데, 실제로 버튼 클릭을 막고 싶다면 button.disabled = true도 병행해주는 것이 좋습니다.
| return emailRegex.test(email); | ||
| } | ||
|
|
||
| function checkValidity() { |
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 상태 변경까지 이 함수에서 동시에 처리하고 있어서 함수 역할이 모호해진 것 같아요.
아래와 같은 방식로 '판단로직'과 '결과반영' 을 분리해주시면 가독성과 유지보수성이 좋아질 것 같습니다.
function isFormValid() {
const emailValid = validateEmail(emailInput.value);
const passwordValid = passwordInput.value.length >= 8;
const noErrors = !emailError.textContent && !passwordError.textContent;
return emailInput.value && passwordInput.value && emailValid && passwordValid && noErrors;
}
function updateButtonState() {
if (isFormValid()) {
loginBtn.classList.remove("disabled");
loginBtn.disabled = false;
} else {
loginBtn.classList.add("disabled");
loginBtn.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.
login.js와 signup.js 에 중복 로직이 꽤 있는 것 같습니다. 각각의 파일에서 핵심 로직만 남기고, 공통 처리는 formUtils.js 같은 파일로 분리하면 좋겠네요.
//formUtil.js
export function isValidEmail(email) {
return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email);
}
export function setError(inputEl, errorEl, message) {
errorEl.textContent = message;
inputEl.classList.add("input-error");
}
export function clearError(inputEl, errorEl) {
errorEl.textContent = "";
inputEl.classList.remove("input-error");
}
export function updateButtonState(buttonEl, isValid) {
buttonEl.disabled = !isValid;
buttonEl.classList.toggle("disabled", !isValid);
}
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
멘토에게