-
Notifications
You must be signed in to change notification settings - Fork 26
[이재원] sprint4 #71
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
The head ref may contain hidden characters: "Basic-\uC774\uC7AC\uC6D0-sprint4"
[이재원] sprint4 #71
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.
첫 JS 미션 수행하시느라 고생많으셨습니다! 👍👍
질문 주신 내용에 대해서는 인라인 코멘트로 리뷰 남겼으니 참고 부탁드립니다.
function handleFocustOutId() { | ||
const emailPattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
const value = $userId.value.trim(); | ||
if (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.
$xxxError.style.display = 'block';
$xxxError.innerText = 'xxx를 입력해주세요.';
$xxx.style.border = '2px solid red';
xxxCheck = false;
위와 같은 코드 라인이 이메일, 닉네임, 비밀번호 등에서 계속 반복되고 있습니다.
아래와 같이 추상화해서 중복 코드를 제거하면 좋을 것 같습니다.
function showError(input, errorEl, message) {
errorEl.style.display = 'block';
errorEl.innerText = message;
input.style.border = '2px solid red';
}
function clearError(input, errorEl) {
errorEl.style.display = 'none';
input.style.border = '';
}
let nicknameCheck = false; | ||
let passwordAuthCheck = false; | ||
|
||
function btnCheck() { |
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.
함수명은 보다 직관적이게, 그리고 가급적 동사로 시작되도록 지어주시는게 좋습니다!
updateSubmitButtonState
정도로 수정하면 좋을것 같네요.
} | ||
} | ||
|
||
function handleFocustOutId() { |
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.
이메일 인풋과 관련된 이벤트핸들러이니 handleFocuseOutEmail
이 더 적합할 것 같네요.
const $viewIcon = document.querySelector('#viewIcon'); | ||
const $viewIcon2 = document.querySelector('#viewIcon2'); | ||
let idCheck = false; | ||
let passwordCheck = false; |
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.
passwordCheck라는 변수명이 비밀번호 일치 여부인지, 비밀번호 유효성인지 불명확한 것 같습니다. isPasswordValid, isPasswordConfirmed 등으로 명확히 분리하면 좋을 것 같습니다.
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 에서 아래와 같은 유틸 함수가 중복되어 있습니다.
- 이메일 형식 검사
- 비밀번호 길이 검사
- 값이 비어있는지 확인하는 로직
이러한 구조는 유지보수에 불리하고, 검증 기준이 바뀔 경우 양 파일을 모두 수정해야 하므로 실수를 유발하기 쉽습니다. 이런 유효성 판단 함수는 아래와 같이 ** 공통 유틸 함수**로 분리 하면 좋을 것 같네요.
// validation.js 등으로 분리
export function showError(input, errorEl, message) { ... }
export function clearError(input, errorEl) { ... }
export function isEmailValid(value) { ... }
export function isNotEmpty(value) { ... }
</div> | ||
<div class="form-group"> | ||
<label for="password">비밀번호</label> | ||
<div class="input-wrapper"> | ||
<input id="password" type="password" name="password" /> | ||
<img src="images/password-icon1.png" alt="보기 아이콘" /> | ||
<img id="viewIcon" src="images/eye-close.png" alt="보기 아이콘" /> | ||
<p id="passwordError" style="display: none; color: red"></p> |
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에서 직접 인라인 스타일을 조작하고 계신데요, 이는 간단한 방법이긴 하지만 CSS 클래스와 충돌하거나 덮어쓰기 위험이 있어 유지보수에 불리합니다.
차라리 아래와 같이 에러 메시지용 클래스를 정의하고, js에서는 클래스 추가/제거만 담당하는 구조로 수정해보시면 어떨까요??
/* CSS */
.error-message {
display: none;
color: red;
font-size: 14px;
}
.error-message.visible {
display: block;
}
|
||
function handleFocustOutId() { | ||
const emailPattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
const value = $userId.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() 메소드 활용 좋습니다! 👍
function handlePassWordView() { | ||
const isPassWordVisible = $password.type === 'text'; | ||
$password.type = isPassWordVisible ? 'password' : 'text'; | ||
$viewIcon.src = isPassWordVisible |
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.
src 컨트롤로 토글 버튼 구현해주신 점 좋네요!👏
|
|
요구사항
기본
로그인
-회원가입
심화
주요 변경사항
스크린샷
멘토에게