-
Notifications
You must be signed in to change notification settings - Fork 15
[김현진] sprint4 #37
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 #37
The head ref may contain hidden characters: "Basic-\uAE40\uD604\uC9C4-sprint4"
Conversation
- 문구 추가에 따른 일부 margin, padding 조정
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주차 작업 수고하셨습니다~
심화 요구사항까지 구현하시는 것이 쉽지는 않으셨을텐데 대단해요.
다만 input error시 디자인과 다른 것 처럼 보여요. 확인 후 수정해주시면 더 완성도있는 프로젝트가 될 것 같습니다~
프사의 고양이 너무 귀여워요 🐈⬛ 다음 주차도 화이팅이에요!
| 판다마켓이 처음이신가요? <a href="./signup.html">회원가입</a> | ||
| </p> | ||
| </div> | ||
| <script type="module" src="./auth.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 태그는 상단에 있는게 구조 파악측면에서 유리하다고 생각해서 큰 이유가 없다면 상단 head 태그에 두시는 것을 추천드려요~
특히 타입이 모듈인 스크립트는 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 파일을 분리해 관리하시는 걸 추천드립니다.
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.
💊 제안
한 파일에서 최상단에 변수 선언들을 위치시키는 것처럼 일정한 순서로 코드가 작성되는 것이 가독성 측면에서 좋습니다~
지금 변수와 이벤트 바인딩문이 번갈아가며 작성되어 있고 그렇게 위치해야하는 이유도 잘 모르겠어요~
가능하면 이벤트 바인딩문은 변수 선언문보다 하단에 위치시키는 것을 추천드립니다!
| // input 이메일 error 처리 | ||
| const emailNoneError = document.querySelector(".error-message.email-none"); | ||
| const emailWrongError = document.querySelector(".error-message.email-wrong"); |
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를 통해 바꿔주시는 방법으로 작업하시는 것을 추천드려요!
| let passwordValue = ""; | ||
|
|
||
| const handlePasswordFocusout = (e) => { | ||
| passwordValue = e.target.value.trim(); | ||
| if (passwordValue.length === 0) { | ||
| passwordNoneError.classList.add("show"); | ||
| passwordInput.classList.add("error-box"); | ||
| passwordWrongError.classList.remove("show"); | ||
| } else if (!passwordInput.checkValidity()) { | ||
| passwordNoneError.classList.remove("show"); | ||
| passwordInput.classList.add("error-box"); | ||
| passwordWrongError.classList.add("show"); | ||
| } else { | ||
| passwordNoneError.classList.remove("show"); | ||
| passwordWrongError.classList.remove("show"); | ||
| passwordInput.classList.remove("error-box"); | ||
| } | ||
| }; |
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.
💊 제안
passwordValue 변수를 함수 바깥에 두기보다는, 함수 내부에서 선언하는 것이 더 명확할 것 같습니다.
하단의 handlePasswordConfirm 함수에서 재사용하기 위해 이렇게 작성하신 것 같지만,
필요한 시점에 값을 직접 읽어오는 방식이 더 직관적이고 유지보수에도 유리합니다~
| let passwordValue = ""; | |
| const handlePasswordFocusout = (e) => { | |
| passwordValue = e.target.value.trim(); | |
| if (passwordValue.length === 0) { | |
| passwordNoneError.classList.add("show"); | |
| passwordInput.classList.add("error-box"); | |
| passwordWrongError.classList.remove("show"); | |
| } else if (!passwordInput.checkValidity()) { | |
| passwordNoneError.classList.remove("show"); | |
| passwordInput.classList.add("error-box"); | |
| passwordWrongError.classList.add("show"); | |
| } else { | |
| passwordNoneError.classList.remove("show"); | |
| passwordWrongError.classList.remove("show"); | |
| passwordInput.classList.remove("error-box"); | |
| } | |
| }; | |
| const handlePasswordFocusout = (e) => { | |
| const passwordValue = e.target.value.trim(); | |
| if (passwordValue.length === 0) { | |
| passwordNoneError.classList.add("show"); | |
| passwordInput.classList.add("error-box"); | |
| passwordWrongError.classList.remove("show"); | |
| } else if (!passwordInput.checkValidity()) { | |
| passwordNoneError.classList.remove("show"); | |
| passwordInput.classList.add("error-box"); | |
| passwordWrongError.classList.add("show"); | |
| } else { | |
| passwordNoneError.classList.remove("show"); | |
| passwordWrongError.classList.remove("show"); | |
| passwordInput.classList.remove("error-box"); | |
| } | |
| }; |
| const handlePasswordView = () => { | ||
| const isHidden = passwordInput.type === "password"; | ||
|
|
||
| if (isHidden) { | ||
| passwordInput.type = "text"; | ||
| passwordViewIcon.classList.add("show"); | ||
| passwordNoViewIcon.classList.remove("show"); | ||
| } else { | ||
| passwordInput.type = "password"; | ||
| passwordNoViewIcon.classList.add("show"); | ||
| passwordViewIcon.classList.remove("show"); | ||
| } | ||
| }; |
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.
💬 여담
아래처럼 toggle method를 이용해서도 구현할 수 있습니다~
| const handlePasswordView = () => { | |
| const isHidden = passwordInput.type === "password"; | |
| if (isHidden) { | |
| passwordInput.type = "text"; | |
| passwordViewIcon.classList.add("show"); | |
| passwordNoViewIcon.classList.remove("show"); | |
| } else { | |
| passwordInput.type = "password"; | |
| passwordNoViewIcon.classList.add("show"); | |
| passwordViewIcon.classList.remove("show"); | |
| } | |
| }; | |
| const handlePasswordView = () => { | |
| const isHidden = passwordInput.type === "password"; | |
| passwordInput.type = isHidden ? "text" : "password"; | |
| passwordViewIcon.classList.toggle("show", isHidden); | |
| passwordNoViewIcon.classList.toggle("show", !isHidden); | |
| }; |
| if (passwordConfirmNoViewIcon) { | ||
| passwordConfirmNoViewIcon.addEventListener( | ||
| "click", | ||
| handlePasswordConfirmView | ||
| ); | ||
| } |
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.
💬 여담
변수가 null 또는 undefined일 때만 안전하게 넘어가도록 하려면 optional chaining을 활용하는 것도 좋은 방법입니다.
이렇게 작성하면 존재 여부를 따로 조건문으로 확인하지 않으므로 코드가 조금 더 간결해집니다.
| if (passwordConfirmNoViewIcon) { | |
| passwordConfirmNoViewIcon.addEventListener( | |
| "click", | |
| handlePasswordConfirmView | |
| ); | |
| } | |
| passwordConfirmNoViewIcon?.addEventListener( | |
| "click", | |
| handlePasswordConfirmView | |
| ); |
|
|
||
| const handlePasswordConfirm = (e) => { | ||
| const passwordConfirmValue = e.target.value.trim(); | ||
| console.log(passwordConfirmValue); |
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.
❗️ 수정요청
개발시 확인을 위해서 넣어주신 것 같아요! 확인 후에는 지워주시는 것을 추천드려요.
| console.log(passwordConfirmValue); |
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.
💊 제안
회원가입 페이지에서 비밀번호 입력 후 비밀번호 확인 입력하고 다시 비밀번호를 변경하면 비밀번호 확인 인풋은 재확인 로직을 실행하지 않네요~ 해당 로직에서 비밀번호 확인 인풋 값도 재검증하면 해결될 것 같습니다~
| <div class="pw-wrapper"> | ||
| <div class="input-contain"> | ||
| <label for="join-nickname">닉네임</label> | ||
| <input |
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.
❗️ 수정요청
닉네임 input의 경우 required 속서이 없어서, auth.js에서 form.checkValidity 함수로 유효성 검사시 통과하게 되네요~
추후 로그인, 회원가입 js를 분리하시면서 이 부분도 고민해보시면 좋겠습니다.
요구사항
기본
로그인
회원가입
심화
주요 변경사항
주강사님에게