-
Notifications
You must be signed in to change notification settings - Fork 39
[이태식] sprint4 #164
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 #164
The head ref may contain hidden characters: "Basic-\uC774\uD0DC\uC2DD-sprint4"
Conversation
input-valid클래스 추가 및 제거
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 미션도 화이팅입니다.
-
입력하는 즉시 비밀번호 유효성 검사를 하고 에러 메세지나 버튼 활성화를 하기 위해 이벤트를 input으로 해주거나 passwordCheck.blur()를 시도해봤는데 생각처럼 되지 않아 우선 이벤트를 focusout 으로 구현했습니다.:
어떤 시점의 유효성 검사를 할지는 기획에 따른 거라 생각해 요구사항이 명확히 없으면 선택이라고 생각합니다~ foucsout시 검사하는 것도 좋습니다! 다만 의도하셨던 바가 있다고 하시니 나중에 다시 이유를 찾아보시고 재시도해보시면 좋겠네요. -
회원가입과 로그인 페이지를 다루는 동작을 한번에 하려다 보니 코드가 짧아진 부분도 있지만 오히려 복잡해지는 느낌을 받은 거 같습니다.:
저도 동일한 느낌을 받았습니다. 코멘트 드렸으니 참고해서 수정해보시면 좋겠습니다!
| </body> | ||
| </html> | ||
| </main> | ||
| <script 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 태그를 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를 따로 만드시는 것을 추천드려요!
| return; | ||
| } | ||
| if (event.target.value === "") { | ||
| event.target.nextElementSibling?.remove(); |
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.
💊 제안
이렇게 접근하시는 것도 나쁘지 않지만 이는 HTML 구조와 긴밀하게 연관되게 되므로,
HTML에서 위치만 변경되어도 작동되지 않을 위험이 커집니다~
가능하면 부모요소로 접근해 해당 요소 내에서 재탐색을 해보세요.
| const loginForm = document.querySelector(".login-form"); | ||
| const signupForm = document.querySelector(".signup-form"); | ||
|
|
||
| const setInputError = (event) => { |
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.
❗️ 수정요청
한 함수안에서 모든 인풋의 에러처리를 하고 있어 가독성이 많이 떨어집니다!
인풋 별 에러처리 로직을 분리하셔서 가독성을 높여보세요.
const setInputError = (event) => {
if (event.target.closest("button")) return;
const { id, value, validationMessage, nextElementSibling } = event.target;
nextElementSibling?.remove();
if (id === "useremail") {
handleUserEmailError(event.target, validationMessage);
return;
}
if (id === "password") {
handlePasswordError(event.target, value);
return;
}
if (id === "passwordCheck") {
handlePasswordCheckError(event.target, value);
return;
}
handleDefaultError(event.target, value);
};| if (type === "empty") { | ||
| if (!element.nextElementSibling) { |
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.
💊 제안
if문의 중첩은 가독성에 좋지 않으니 지금의 경우 아래처럼 하시면 더 좋을 것 같습니다.
| if (type === "empty") { | |
| if (!element.nextElementSibling) { | |
| if (element.nextElementSibling) return; // 이 조건의 해당할 경우 아래 로직이 실행되지 않음 | |
| if (type === "empty") { |
| }; | ||
|
|
||
| const isValidForm = (event) => { | ||
| for (let e of event.currentTarget) { |
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.
💊 제안
for of 문을 쓰실때 재할당하는 것이 아니라면 const 로 선언해주세요!
| for (let e of event.currentTarget) { | |
| for (const target of event.currentTarget) { |
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.
💊 제안
코드는 일정한 순서를 유지하는 것이 가독성을 높이는 데 도움이 됩니다.
현재 코드에서는 변수 선언과 함수 선언이 번갈아가며 작성되어 있어 흐름을 따라가기 어려울 수 있습니다.
일반적으로 변수 선언 → 함수 선언 → 실행 코드 순으로 작성하는 것이 읽기 쉽고 유지보수하기 좋습니다.
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
https://leetaesik-sprint.netlify.app

멘토에게