-
Notifications
You must be signed in to change notification settings - Fork 21
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 #58
The head ref may contain hidden characters: "Basic-\uAE40\uD76C\uC9C4-sprint4"
[김희진] sprint4 #58
Conversation
const emailErrorMsg = getEmailErrorMsg(emailInput.value); | ||
isEmailValid = !emailErrorMsg; | ||
setErrorMessage(emailError, emailErrorMsg ?? ''); | ||
updateErrorDisplay(emailInput, emailError, !!emailErrorMsg); |
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.
gpt추천으로 이렇게 쓰긴했는데, !! 이렇게 하면 boolean으로만 바꿔주는 용도 인것으로 생각되어지는데요..
조건문에서 truthy여부만 판단하믄거라면 사용안하는게 나을까요?
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.
boolean 타입으로 변환하고 싶을 때 이렇게 사용하니 사용법 자체는 괜찮습니다.
다만 isEmailValid라는 boolean 변수가 이미 잇으니 이것을 활용하는게 더 가독성측면에서 좋을 것 같습니다.
updateErrorDisplay(emailInput, emailError, !isEmailValid);
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를 사용한 첫 미션이었는데 잘 작성해주셔서 읽기 편했습니다.
아래는 리뷰한 파일에 대한 전반적인 리뷰 사항입니다.
참고해주세요~
- 동일한 사항에 대해서는 한번만 코멘트를 답니다. 비슷한 경우를 찾아서 변경해보세요~
- 최대한 반복을 피해서 재사용하려고 하신것이 느껴집니다. login, signUp 관련 js를 분리하신 것도 좋습니다. 다만 더 공통화해서 사용할 수 잇는 부분이 보이니 한번 고민해보시면 좋을 것 같습니다.
- 특정 함수에서만 사용된다면 지역변수로 선언하시는것이 코드파악에 더 좋을 것 같습니다.
<input id="password" type="password" name="password" class="form__input" placeholder="비밀번호를 입력해주세요" /> | ||
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" class="visible-icon"> | ||
<input id="password" type="password" name="password" class="form-input" placeholder="비밀번호를 입력해주세요" /> | ||
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" class="invisible-icon" aria-label="비밀번호 보기"> |
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.
P2:
aria-label
은 상호작용이 가능한 요소에 어떤 요소인지 설명하는 속성입니다.
해당 요소는 상호작용이 가능하니 아래처럼 하면 더 좋을 것 같습니다.
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" class="invisible-icon" aria-label="비밀번호 보기"> | |
// button 상태에 따라 JS로 aria-label을 바꿔주면 좋을 것 같습니다. | |
<button type='button' "invisible-icon" aria-label="비밀번호 보기"> | |
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" aria-description='눈 감은 아이콘'> ... </svg> | |
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" aria-description='눈 뜬 아이콘'> ... </svg> | |
</button> |
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.
P3:
상수는 대문자로 작성하시는 것을 권해드립니다.
제 취향이기도하지만 일반적인 경우에도 권장되는 방식입니다~
https://google.github.io/styleguide/javascriptguide.xml?showone=Constants#Constants
export const emailRegex = /^[^s@]+@[^s@]+.[^s@]+$/; | ||
export const emptyEmailMsg = '이메일을 입력해주세요.'; | ||
export const invalidEmailMsg = '잘못된 이메일 형식입니다.'; | ||
export const emptyNicknameMsg = '닉네임을 입력해주세요.'; | ||
export const emptyPasswordMsg = '비밀번호를 입력해주세요.'; | ||
export const invalidPasswordLengthMsg = '비밀번호를 8자 이상 입력해주세요.'; | ||
export const passwordNotMatch = '비밀번호가 일치하지 않습니다..'; | ||
export const passwordMinLength = 8; |
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.
P3:
관련된 친구들끼리 묶어도 좋을 것 같습니다.
export const passwordMinLength = 8;
export const ERROR_MESSAGES = {
EMPTY_EMAIL: '이메일을 입력해주세요.',
INVALID_EMAIL: '잘못된 이메일 형식입니다.',
EMPTY_NICKNAME: '닉네임을 입력해주세요.',
EMPTY_PASSWORD: '비밀번호를 입력해주세요.',
INVALID_PASSWORD_LENGTH: '비밀번호를 8자 이상 입력해주세요.',
PASSWORD_NOT_MATCH: '비밀번호가 일치하지 않습니다..', // 아련한 에러 문구네요~
}
@@ -0,0 +1,87 @@ | |||
import { getEmailErrorMsg, getPasswordErrorMsg, getNicknameErrorMsg } from './validationErrorMessage.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.
P1:
사용하지 않는 것은 지워주세요.
import { getEmailErrorMsg, getPasswordErrorMsg, getNicknameErrorMsg } from './validationErrorMessage.js'; | |
import { getEmailErrorMsg, getPasswordErrorMsg } from './validationErrorMessage.js'; |
|
||
// Elements | ||
const emailInput = document.querySelector('#email'); | ||
const emailError = document.querySelector('#email + .input-validation-error'); |
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.
P1:
지금으로는 크게 헷갈리지 않지만 기존의 변수명인 emailError
의 경우 에러 메시지인지 에러 메시지가 나올 태그인지 알기어렵습니다.
아래처럼 좀 더 명확하게 명명해주시면 좋을 것 같습니다.
(제가 네이밍에는 크게 자신이 없어서 참고만해주세요~)
const emailError = document.querySelector('#email + .input-validation-error'); | |
const emailErrorContainer = document.querySelector('#email + .input-validation-error'); |
if (showError) { | ||
inputElement.classList.add('error'); | ||
errorElement.style.visibility = 'visible'; | ||
} else { | ||
inputElement.classList.remove('error'); | ||
errorElement.style.visibility = 'hidden'; | ||
} |
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.
P3:
아래 코드도 동일 동작합니다.
if (showError) { | |
inputElement.classList.add('error'); | |
errorElement.style.visibility = 'visible'; | |
} else { | |
inputElement.classList.remove('error'); | |
errorElement.style.visibility = 'hidden'; | |
} | |
if (showError) { | |
inputElement.classList.add('error'); | |
errorElement.style.visibility = 'visible'; | |
return; | |
} | |
inputElement.classList.remove('error'); | |
errorElement.style.visibility = 'hidden'; |
visibleIcon.addEventListener('click', togglePasswordVisibility); | ||
invisibleIcon.addEventListener('click', togglePasswordVisibility); | ||
|
||
const form = document.querySelector('.form-login'); |
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.
P1:
변수 선언문은 최상단에 두는 것이 코드파악에 더 좋을 것 같습니다
const emailErrorMsg = getEmailErrorMsg(emailInput.value); | ||
isEmailValid = !emailErrorMsg; | ||
setErrorMessage(emailError, emailErrorMsg ?? ''); | ||
updateErrorDisplay(emailInput, emailError, !!emailErrorMsg); |
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.
boolean 타입으로 변환하고 싶을 때 이렇게 사용하니 사용법 자체는 괜찮습니다.
다만 isEmailValid라는 boolean 변수가 이미 잇으니 이것을 활용하는게 더 가독성측면에서 좋을 것 같습니다.
updateErrorDisplay(emailInput, emailError, !isEmailValid);
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.
P2:
login.js와 겹치는 로직들이 많은 것 같습니다.
가능하면 반복 사용 되는 친구들은 분리해서 작성해주세요.
그래야 어떤 로직을 공유하고, 어떤 로직은 아닌지 파악하기 쉽습니다.
ex: updateErrorDisplay, setErrorMessage
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.
P3:
유효성을 검사하고 관련 에러 메시지를 반환하는 함수끼리 묶어두신거 좋습니다 👍
요구사항
기본
로그인
회원가입
심화