-
Notifications
You must be signed in to change notification settings - Fork 39
[맹은빈]Sprint4 #146
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 #146
The head ref may contain hidden characters: "Basic-\uB9F9\uC740\uBE48-sprint4"
Conversation
GANGYIKIM
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.
은빈님 4번째 미션 제출 고생하셨습니다~
심화 요구사항까지 다 구현하시고 대단하네요.
제가 드린 피드백의 구현은 필수는 아니니 마음에 드시는 것만 반영해주세요~
다음 React 미션도 화이팅입니다!
| class="input-box" | ||
| placeholder="이메일을 입력해주세요" | ||
| /> | ||
| <div id="emailErrDiv" class="errDiv"></div> |
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.
💊 제안
클래스명에 태그명이 들어갈 필요가 없어보여요~
또한 클래스명 규칙도 하나로 통일해주시는 것을 추천드려요!
emailErrorDiv는 카멜케이스 방식, email-error-div는 케밥케이스 방식인데 지금의 경우 혼용되고 있으니 하나로 통일해주세요~
추가로 아이디와 클래스명의 규칙을 구분하시는 것은 괜찮습니다~
| <script src="/scripts/loginValidate.js"></script> | ||
| <script src="/scripts/passwordIcon.js"></script> |
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.
💊 제안
아마 script 태그를 HTML의 하단에 배치하신 이유가 script가 문서의 렌더링을 막지 않도록 하기 위해서이실 것 같아요.
하지만, script 태그에 defer나 async 속성을 사용하면 이런 문제를 해결할 수 있기 때문에 반드시 하단에 배치할 필요는 없습니다!
또한 script 태그는 상단에 있는게 구조 파악에서도 유리하기 때문에 상단 head 태그에 두시는 것을 추천드려요~
지금과 같은 경우 DOM을 조작하는 JS 이니 defer 속성을 추가하시면 되겠습니다~
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 파일과 에러 메시지나 로직 측면에서 동일한 것이 많습니다~
이런 경우 중복을 줄이기 위해 반복 사용되는 함수, 정규식, 에러메시지들은 다른 파일로 분리하시고 각 js 파일에서 가져와서 사용하시는 것을 추천드려요!
| @@ -0,0 +1,116 @@ | |||
| const email = document.getElementById('email'); | |||
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.
💊 제안
email이라는 변수명은 어떤 값인지 알기 어려우므로 emailInput 같이 더 해당값을 잘 설명하는 이름으로 바꿔주시는 것을 추천드립니다.
|
|
||
| function validateEmail() { | ||
| const emailValue = email.value.trim(); | ||
| const emailPattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; |
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.
💊 제안
scripts/loginValidate.js 파일에서도 동일한 정규식을 사용하고 있는데 그때마다 선언해서 쓰면 문제가 있을 것 같아요.
정규식을 반복해서 선언하면 유지보수가 어려워지고, 코드의 일관성이 떨어질 수 있습니다. 만약 정규식을 수정해야 할 때 여러 곳에서 변경해야 하고 실수로 서로 다른 정규식을 사용할 위험도 커집니다.
이렇게 두개의 파일에서 사용하고 있으니 해당 정규식과 test 함수를 따른 파일로 분리해 사용하시는 것을 추천드려요!
| if (confirmValue === '') { | ||
| confirmError.textContent = '비밀번호를 입력해주세요.'; | ||
| confirmPassword.classList.add('invalid'); | ||
| confirmPasswordCheck = false; | ||
| } | ||
|
|
||
| if (confirmValue !== passwordValue) { | ||
| confirmError.textContent = '비밀번호가 일치하지 않습니다.'; | ||
| confirmPassword.classList.add('invalid'); | ||
| confirmPasswordCheck = false; | ||
| } else { | ||
| confirmError.textContent = ''; | ||
| confirmPassword.classList.remove('invalid'); | ||
| confirmPasswordCheck = 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.
❗️ 수정요청
지금 로직에 따르면 confirmValue와 passwordValue가 빈값일때 '비밀번호를 입력해주세요.' 라는 에러메시지가 나올 수가 없습니다.
아래의 if (confirmValue !== passwordValue) 에서 동일하기 때문에 else 문을 타서 valid하다고 판단되기 때문입니다.
아래처럼 작성하셔야 의도대로 동작할 것 같습니다~
| if (confirmValue === '') { | |
| confirmError.textContent = '비밀번호를 입력해주세요.'; | |
| confirmPassword.classList.add('invalid'); | |
| confirmPasswordCheck = false; | |
| } | |
| if (confirmValue !== passwordValue) { | |
| confirmError.textContent = '비밀번호가 일치하지 않습니다.'; | |
| confirmPassword.classList.add('invalid'); | |
| confirmPasswordCheck = false; | |
| } else { | |
| confirmError.textContent = ''; | |
| confirmPassword.classList.remove('invalid'); | |
| confirmPasswordCheck = true; | |
| } | |
| if (confirmValue === '') { | |
| confirmError.textContent = '비밀번호를 입력해주세요.'; | |
| confirmPassword.classList.add('invalid'); | |
| confirmPasswordCheck = false; | |
| }else if (confirmValue !== passwordValue) { | |
| confirmError.textContent = '비밀번호가 일치하지 않습니다.'; | |
| confirmPassword.classList.add('invalid'); | |
| confirmPasswordCheck = false; | |
| } else { | |
| confirmError.textContent = ''; | |
| confirmPassword.classList.remove('invalid'); | |
| confirmPasswordCheck = true; | |
| } |
| const confirmError = document.getElementById('confirmErrDiv'); | ||
| const signupButton = document.getElementById('signupButton'); | ||
|
|
||
| let emailCheck = 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.
❗️ 수정요청
isSignUpPossible 처럼 boolean 값임으로 알 수 있도록 isEmailValid 같은 이름을 추천드립니다.
| function signUpCheck() { | ||
| if (emailCheck && nicknameCheck && passwordCheck && confirmPasswordCheck) { | ||
| isSignUpPossible = true; | ||
| signupButton.classList.add('activate'); | ||
| } else { | ||
| signupButton.classList.remove('active'); | ||
| isSignUpPossible = 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.
💬 여담
아래 코드도 동일동작합니다~
| function signUpCheck() { | |
| if (emailCheck && nicknameCheck && passwordCheck && confirmPasswordCheck) { | |
| isSignUpPossible = true; | |
| signupButton.classList.add('activate'); | |
| } else { | |
| signupButton.classList.remove('active'); | |
| isSignUpPossible = false; | |
| } | |
| } | |
| function signUpCheck() { | |
| const isFormValid = emailCheck && nicknameCheck && passwordCheck && confirmPasswordCheck | |
| isSignUpPossible = isFormValid; | |
| signupButton.classList.toggle('activate', isFormValid); | |
| } |
| } | ||
| } | ||
|
|
||
| function SignUp() { |
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에서 일반적으로 함수는 대문자로 시작하지 않습니다. 주로 class(css 클래스가 아닙니다~)의 경우나 생성자를 가진 객체의 경우 대문자로 시작하게 작성합니다~
따라서 일반적인 함수의 경우 signUp, onSumbit과 같은 이름으로 변경하시는 것을 추천드립니다.
요구사항
기본
로그인
회원가입
심화
주요 변경사항
배포링크
-https://bespoke-sfogliatella-a26ba3.netlify.app/
스크린샷
멘토에게