Conversation
|
회원가입 페이지도 하는 줄 모르고 로그인에 투자 다 했습니다...ㅠ |
There was a problem hiding this comment.
가희님 이번주차 스프린트 미션 고생하셨습니다~
로그인에 전력을 다하셨다고 하셨는데 회원가입이랑 로직이 비슷하니 금방 하실 수 있어요~ 😄
이번에 코드리뷰 받으신거 바탕으로 나중에 추가해보시면 더 재미있게 하실 수 있슴다.
아래는 올려주신 파일 변경사항에서 전반적으로 해당하는 내용들입니다.
- 동일한 사항에 대해서는 한번만 코멘트를 답니다. 비슷한 경우를 찾아서 변경해보세요~
- 변수명, 함수명에 대해 조금더 고민해보시면 좋겠습니다. emailInputElement과 같은 변수명의 경우는 input이 이미 태그라는 것을 나타내기 때문에 emailInput 정도로 변경하셔도 좋을 것 같고, 그와 달리 errorMessage와 같은 경우 실제로는 에러타입에 따른 에러 문구가 아닌, 해당 문구가 들어갈 요소를 담은 변수인데 이가 나타나지 않으니 errorMessgaeElement | errorMessageContainer와 같은 식으로 네이밍하시면 더 코드파악하기 좋을 것 같습니다.
코드를 짤 때 비슷한 로직들이 많아서 더 깔끔하게 만드는 방법이 있는지 궁금합니다!: 기능 구현이 되고 나면 리팩토링을 하고 싶어지죠..! 우선 코드에서 어떤 부분이 마음에 안 드시는지 파악을 해보시고, 이를 바꿔나가시면 좋을 것 같습니다. 제가 생각하는 깔끔한 코드란 가독성이 좋은 코드인데, 가희님은 어떤것이 마음에 안 드시는지 생각해보세요~ 일반적으로 변수, 함수명을 유의미하게 바꾸기, 코드 가독성 개선하기, 관심사 분리하기, DRY 원칙같이 반복 줄이기 등을 수행해 코드의 가독성을 향상시킵니다. 다양한 개선 방향이 있고 이는 가희님께서 하실 수 있는 만큼만 하면 되는 것이니 너무 부담가지지 않으셔도 됩니다. 나중에 시간이 지나시면 코드 보는 눈이 더 생기고 좋은 방향성을 알게 되면서 어느정도 해결될 문제이기도 합니다
| </form> | ||
| <button class="login-btn" onclick="location.href='/login'">로그인</button> | ||
| <button id="login" class="login-btn" onclick="location.href='/items'">로그인</button> |
There was a problem hiding this comment.
P3:
해당 button은 disabled true로 해두는 것이 동작상 좋지 않을까요? 🧐
처음에는 input들이 비어있어서 invalid할테니까용
| <button id="login" class="login-btn" onclick="location.href='/items'">로그인</button> | |
| <button id="login" disabled class="login-btn" onclick="location.href='/items'">로그인</button> |
| @@ -57,5 +61,6 @@ | |||
| <a href="login.html">로그인</a> | |||
| </section> | |||
| </main> | |||
| <script src="signup.js"></script> | |||
There was a problem hiding this comment.
P2:
HTML 문서 다운로드 후 JS가 다운로드 되게 하단에 배치하신 것 같은데
하단에 배치된 스크립트는 구문분석 순서상 추후에 다운로드 되게 만들어주지만 일반적으로 스크립트 태그가 위치되는 head 요소가 아니므로 가독성 측면에서 단점이 있고 이러한 동작은 스크립트 자체 속성을 통해 이끌어 낼 수 있으므로 script async, defer 속성 사용을 추천드립니다.
https://developer.mozilla.org/ko/docs/Web/HTML/Element/script#defer
https://ko.javascript.info/script-async-defer
| emailInputElement.addEventListener('blur', emailForm) | ||
| emailInputElement.addEventListener('focus', removeMessage) | ||
|
|
||
| pwInputElement.addEventListener('blur', pwForm) | ||
| pwInputElement.addEventListener('focus', removeMessage) |
There was a problem hiding this comment.
P1:
이벤트를 연결하는 코드들이 함수 선언문 사이사이에 들어가 있어서 코드 파악이 어렵습니다.
가능하면 변수 선언 => 함수 선언 => 이벤트 바인딩문 과 같은 순서처럼 관련있는 코드끼리 기준을 정해서 묶어 작성하시는 것을 추천드립니다.
| const errorMessage = document.querySelector('.error-message'); | ||
| const pwErrorMessage = document.querySelector('.error-message-pw'); | ||
|
|
||
| function emailForm(e) { |
There was a problem hiding this comment.
P1:
변수명 함수명은 좀 더 유의미하고 어떤 동작을 하는 지 알 수 있는 이름이면 좋겠습니다.
해당 함수에서 인자 e는 사용되지 않으니 지워주시는게 더 좋겠습니다.
| function emailForm(e) { | |
| function checkEmailValidation() { |
There was a problem hiding this comment.
P3:
아래처럼도 작성 가능합니다.
const checkEmailFormatValid = (value: string) => {
return !(value.includes("@") && value.includes("."))
}
function checkEmailValidation(e) {
const input = e.currentTarget;
const value = input.value.trim();
if (!value) {
emailInputElement.classList.add('error');
errorMessage.textContent ="이메일을 입력해주세요."
errorMessage.style.display = 'block';
return ;
}
if (checkEmailFormatValid(value)) {
emailInputElement.classList.add('error');
errorMessage.textContent ="잘못된 이메일 형식입니다."
errorMessage.style.display = 'block';
return;
}
emailInputElement.classList.remove('error');
errorMessage.style.display = 'none';
}지금은 에러종류가 많지 않지만 나중에 에러종류가 많은 경우가 생긴다면
아래처럼 작성할 수도 있습니다.
const getEmailError = (value: string): {type: EMAIL_ERROR_TYPE} => {
/** 이메일 input의 value를 받아 해당하는 에러 타입을 반환하는 함수 */
}
function checkEmailValidation(e) {
const input = e.currentTarget;
const errorType = getEmailError(input.value.trim());
if (errorType) {
emailInputElement.classList.add('error');
errorMessage.textContent =ErrorMessage[errorType]
errorMessage.style.display = 'block';
return ;
}
emailInputElement.classList.remove('error');
errorMessage.style.display = 'none';
}| pwInputElement.addEventListener('blur', pwForm) | ||
| pwInputElement.addEventListener('focus', removeMessage) | ||
|
|
||
| // ---------------------------------------------------------- |
There was a problem hiding this comment.
P3:
어떤 기준으로 코드를 양분하신 건지 주석을 달아주시면 좋을 것 같습니다.
| if(emailInputElement.classList.contains('error') | ||
| || pwInputElement.classList.contains('error')){ | ||
| loginElement.style.backgroundColor = "#9CA3AF" | ||
|
|
||
| } else if(emailInputElement.value === '' || pwInputElement.value === ''){ | ||
| loginElement.style.backgroundColor = "#9CA3AF" | ||
| }else{ | ||
| loginElement.style.backgroundColor = "#3692FF" | ||
| } |
There was a problem hiding this comment.
P2:
조건문의 조건을 변수로 저장해주시면 변수명을 통해 코드를 이해하기 더 쉬워집니다.
| if(emailInputElement.classList.contains('error') | |
| || pwInputElement.classList.contains('error')){ | |
| loginElement.style.backgroundColor = "#9CA3AF" | |
| } else if(emailInputElement.value === '' || pwInputElement.value === ''){ | |
| loginElement.style.backgroundColor = "#9CA3AF" | |
| }else{ | |
| loginElement.style.backgroundColor = "#3692FF" | |
| } | |
| const isError = | |
| emailInputElement.classList.contains("error") || | |
| pwInputElement.classList.contains("error"); | |
| const isEmpty = emailInputElement.value === "" || pwInputElement.value === ""; | |
| if (isError || isEmpty) { | |
| loginElement.style.backgroundColor = "#9CA3AF"; | |
| return; | |
| } | |
| loginElement.style.backgroundColor = "#3692FF"; |
| const nickInputElement = document.getElementById('nickname'); | ||
| const pwInputElement = document.getElementById('pw'); | ||
| const pwCheckInputElement = document.getElementById('pwcheck'); | ||
| const errorMessage = document.querySelector('.error-message'); |
There was a problem hiding this comment.
P2:
해당 변수도 errorMessage가 들어갈 요소이니 아래와 같은 이름이 더 유의미할 것 같습니다.
| const errorMessage = document.querySelector('.error-message'); | |
| const errorMessageElement = document.querySelector('.error-message'); |
요구사항
기본
로그인
이메일 input에서 focus out 할 때, 값이 없을 경우 input에 빨강색 테두리와 아래에 “이메일을 입력해주세요.” 빨강색 에러 메세지를 보입니다.
이메일 input에서 focus out 할 때, 이메일 형식에 맞지 않는 경우 input에 빨강색 테두리와 아래에 “잘못된 이메일 형식입니다” 빨강색 에러 메세지를 보입니다.
비밀번호 input에서 focus out 할 때, 값이 없을 경우 아래에 “비밀번호를 입력해주세요.” 에러 메세지를 보입니다
비밀번호 input에서 focus out 할 때, 값이 8자 미만일 경우 아래에 “비밀번호를 8자 이상 입력해주세요.” 에러 메세지를 보입니다.
input 에 빈 값이 있거나 에러 메세지가 있으면 ‘로그인’ 버튼은 비활성화 됩니다.
input 에 유효한 값을 입력하면 ‘로그인' 버튼이 활성화 됩니다.
활성화된 ‘로그인’ 버튼을 누르면 “/items” 로 이동합니다
-회원가입
이메일 input에서 focus out 할 때, 값이 없을 경우 input에 빨강색 테두리와 아래에 “이메일을 입력해주세요.” 빨강색 에러 메세지를 보입니다.
이메일 input에서 focus out 할 때, 이메일 형식에 맞지 않는 경우 input에 빨강색 테두리와 아래에 “잘못된 이메일 형식입니다” 빨강색 에러 메세지를 보입니다.
닉네임 input에서 focus out 할 때, 값이 없을 경우 input에 빨강색 테두리와 아래에 “닉네임을 입력해주세요.” 빨강색 에러 메세지를 보입니다.
비밀번호 input에서 focus out 할 때, 값이 없을 경우 아래에 “비밀번호를 입력해주세요.” 에러 메세지를 보입니다
비밀번호 input에서 focus out 할 때, 값이 8자 미만일 경우 아래에 “비밀번호를 8자 이상 입력해주세요.” 에러 메세지를 보입니다.
비밀번호 input과 비밀번호 확인 input의 값이 다른 경우, 비밀번호 확인 input 아래에 “비밀번호가 일치하지 않습니다..” 에러 메세지를 보입니다.
input 에 빈 값이 있거나 에러 메세지가 있으면 ‘회원가입’ 버튼은 비활성화 됩니다.
input 에 유효한 값을 입력하면 ‘회원가입' 버튼이 활성화 됩니다.
활성화된 ‘회원가입’ 버튼을 누르면 “/signup” 로 이동합니다
심화
주요 변경사항
스크린샷
멘토에게