[노준서] sprint4#82
Hidden character warning
Conversation
css/form.css
Outdated
| .password-input input, | ||
| .confirm-input input { | ||
| flex: 1; | ||
| border: none; |
There was a problem hiding this comment.
border: 1.5px solid transparent; 로 수정했습니다
css/form.css
Outdated
| border: none; | ||
| border-radius: 12px; | ||
| background-color: var(--background-color); | ||
| padding: 16px 24px; |
There was a problem hiding this comment.
padding: 16px 24px; 값을
.password-input input,
.confirm-input input { } 으로 옮겼습니다.
padding: 0;
position: relative; 를 추가했습니다.
| font-family: Pretendard, sans-serif; | ||
| } | ||
|
|
||
| .toggle-password { |
There was a problem hiding this comment.
position: absolute;
right: 16px;
추가했습니다
|
파일은 수정 완료했는데 PR을 다시 올려야하나요? 지우고 다시 올리려했는데 지우는 법을 모르겠어서 코멘트로 수정했다고만 올렸습니다... |
baeggmin
left a comment
There was a problem hiding this comment.
첫 js 미션 수행하느라 고생 많으셨습니다!👍👍
| @@ -0,0 +1,202 @@ | |||
| document.addEventListener("DOMContentLoaded", function () { | |||
| // 공통 함수 | |||
| function validateEmail(value) { | |||
There was a problem hiding this comment.
이메일, 비밀번호, 닉네임 등 검증 로직을 validateXXX 함수로 분리한 점 좋습니다 👍
역할 별로 명확하게 구분되었네요!
| if (!value) return "닉네임을 입력해주세요."; | ||
| return ""; | ||
| } | ||
| function validatePasswordCheck(password, passwordCheck) { |
There was a problem hiding this comment.
지금 함수명보다는 validatePasswordConfirmation 가 더 명확할 것 같습니다.
| document.addEventListener("DOMContentLoaded", function () { | ||
| // 공통 함수 | ||
| function validateEmail(value) { | ||
| if (!value) return "이메일을 입력해주세요."; |
There was a problem hiding this comment.
공백만 입력한 경우를 대비해서 value.trim() 메소드를 이용해봐도 좋을 것 같아요.
| function updateLoginButtonState() { | ||
| const emailErr = validateEmail(emailInput.value); | ||
| const pwErr = validatePassword(passwordInput.value); | ||
| if (!emailInput.value || !passwordInput.value || emailErr || pwErr) { |
There was a problem hiding this comment.
!emailInput.value || !passwordInput.value 이 조건은 validateEmail 과 validatePassword에서 이미 걸러지지 않나요? 🤔
There was a problem hiding this comment.
아 그러네요 ㅎ
수정완료했습니다:)
| const err = validateEmail(emailInput.value); | ||
| emailError.textContent = err; | ||
| emailInput.classList.toggle("error", !!err); | ||
| updateLoginButtonState(); |
There was a problem hiding this comment.
'input' 이벤트핸들러에서 updateLoginButtonState가 호출되고 있어서 'blur'에서는 호출되지 않아도 될 것 같다는 생각이 듭니다. blur 이벤트에서는 검증과 에러메세지 출력만 해도 될 것 같아요.
There was a problem hiding this comment.
updateLoginButtonState(); 삭제 완료했습니다!
| loginButton.disabled = true; | ||
| } | ||
|
|
||
| // ===== 회원가입 ===== |
There was a problem hiding this comment.
위 로그인 로직에 남긴 리뷰들이 회원가입 로직에도 동일하게 적용됩니다.
|
|
||
| signupForm.addEventListener("submit", function (e) { | ||
| e.preventDefault(); | ||
| if (!signupButton.disabled) { |
There was a problem hiding this comment.
UI 상태에 의존한 조건은 지양해주시는게 좋습니다. 지금은 간단한 기능이라 문제없지만, 조건이 조금만 복잡해져도 상태가 변경된 이유를 알기 어려워 버그 가능성이 커질 수 있습니다. ValidateEmail과 validatePassword 의 반환값으로 조건을 거는게 더 좋아보려요.
There was a problem hiding this comment.
const emailErr = validateEmail(emailInput.value);
const nicknameErr = validateNickname(nicknameInput.value);
const passwordErr = validatePassword(passwordInput.value);
const passwordCheckErr = validatePasswordCheck(
passwordInput.value,
passwordCheckInput.value
);
if (!emailErr && !nicknameErr && !passwordErr && !passwordCheckErr) {
window.location.href = "/login.html";
}이런식으로 수정하면 될까요?
|
제가 리뷰 남기기 전이라면 자유롭게 PR 수정하셔도 괜찮습니다~! (코멘트 남기지 않아도 괜찮아요😊) |
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
멘토에게