[이승민] Sprint4 #151
[이승민] Sprint4 #151
Conversation
ghost
requested a review
from 
ghost
added
the
GANGYIKIM
left a comment
GANGYIKIM
left a comment
There was a problem hiding this comment.
승민님 너무 괴로우셨는데도 미션 제출하시다니 너무 고생하셨습니다.
처음 배우는 지식을 이용해서 구현하시는 것이 쉽지는 않은데 이를 스스로 하신 것만해도 대단합니다.
너무 괴롭다고 하셔서 코드 리뷰 드릴때도 최대한 쉽게 남겼습니다. 또한 코멘트의 반영이 필수는 아니니 가능하신 것만 반영하셔도 됩니다~
다음 React 미션도 화이팅입니다.
-
구글링의 한계를 느끼는 경험이였습니다..혹시 저는 그냥 velog나 mdn참고 많이 하는데 혹시 이 방법말고 멘토님이라면 어떻게 질문하고 찾는지 선생님만에 꿀팁이 있을까요...?:
저도 mdn을 좋아합니다. 다만 지금 시점에서 구글링의 한계를 느끼신다면 자료가 없기보다 질문을 어떻게 해야하는지에 대해 어려움을 겪고 계실 것 같습니다. 특정 상황에 대해 뭐라고 검색해야하는지 어려운 것은 지금 시점에서 당연한 것입니다. 이는 시간이 지나고 명확한 용어를 알게 되셔야 좋아질 가능성이 크니 지금은 이러한 상황을 GPT에게 말해서 관련 키워드나 질문을 만들어 달라고 하셔서 구글링하시는 것이 좋을 것 같습니다~ -
처음으로 주석을 달아보았습니다..이게 코드양이 길어지고 Js파일을 제 스스로가 봐도 보기 힘들다보니 gpt한테 이때 어떤 방법을 쓰니?? 라고 물어보니 주석을 쓰라고 하더라고요..네 다음부터 주석을 적절히 잘 활용해야겠다라는 생각이 들었습니다:
관련해서 코멘트 남겼으니 확인해보세요! 주석도 좋은 방법입니다만 로직을 분리하시는 것이 더 좋습니다. -
gpt도움 많이 받았습니다..근데 코드를 그냥 복붙하고 따라쳤다기보다는 일단 이해는 하고 그때그때 마다 찾아쓸 수 있을 정도로 이해했습니다 혹시 추가적으로 추천하는 방법이 있나요..??:
GPT 가 생성해준 코드를 이해하고 작성하셨다면 충분할 것 같습니다. -
혹시 js관련 로직이 제가 지금 많이 부족한데 추천하는 공부나 참고영상 등등 있으면 다 해보겠습니다!!:
코드잇 커리큘럼을 따라가시고 미션을 진행하시고 남는 시간이 있으시다면 각자 공부법에 따라 하시면 된다고 생각합니다~ 책이 편하신 분들은 JS 관련 책을 읽으시고, 영상 매체가 편한 분들은 추가 제공된 학습자료를 보시면 될 것 같습니다. 또한 지금의 승민님 상황과 학습 수준에 따라 달라지므로 이러한 것은 멘토분과 상의해보시거나 추후 저와 zoom에서 얘기해보는 것이 좋을 것 같아요.
| </div> | ||
| </div> | ||
|
|
||
| <script src="scripts/auth.js"></script> |
There was a problem hiding this comment.
💊 제안
아마 script 태그를 HTML의 하단에 배치하신 이유가 script가 문서의 렌더링을 막지 않도록 하기 위해서이실 것 같아요.
하지만, script 태그에 defer나 async 속성을 사용하면 이런 문제를 해결할 수 있기 때문에 반드시 하단에 배치할 필요는 없습니다!
또한 script 태그는 상단에 있는게 구조 파악에서도 유리하기 때문에 상단 head 태그에 두시는 것을 추천드려요~
지금과 같은 경우 DOM을 조작하는 JS 이니 defer 속성을 추가하시면 되겠습니다~
| const button = form.querySelector("button.auth-button"); | ||
| button.disabled = true; |
There was a problem hiding this comment.
❗️ 수정요청
JS 가 아니라 html 에서 button에 disabled 속성을 true로 주고 시작하는게 더 좋을 것 같아요.
JS에서 이렇게 해야할 이유를 잘 모르겠고 이렇게 되면 HTML 에서 button이 활성화된채로 렌더링됩니다~
| function checkFormValidity() { | ||
| let valid = true; | ||
| const inputs = form.querySelectorAll("input"); | ||
| inputs.forEach(function (input) { | ||
| if (input.value.trim() === "") { | ||
| valid = false; | ||
| } | ||
| }); | ||
| if (form.querySelectorAll(".error-message").length > 0) { | ||
| valid = false; | ||
| } | ||
| button.disabled = !valid; | ||
| } |
There was a problem hiding this comment.
💊 제안
폼의 유효성을 검사하는 로직을 따로 분리해주신 것은 좋습니다.
다만 이미 validateEmail과 같은 각 인풋별 유효성 검사 함수를 선언해두셨으니 이를 이용해서 유효성 검사를 하시거나
유효하지 않다면 error-message 라는 클래스를 가진 태그가 폼안에 존재할테니 그냥 해당 여부만 확인하시면 될 것 같습니다!
| function checkFormValidity() { | |
| let valid = true; | |
| const inputs = form.querySelectorAll("input"); | |
| inputs.forEach(function (input) { | |
| if (input.value.trim() === "") { | |
| valid = false; | |
| } | |
| }); | |
| if (form.querySelectorAll(".error-message").length > 0) { | |
| valid = false; | |
| } | |
| button.disabled = !valid; | |
| } | |
| function checkFormValidity() { | |
| const isValid = form.querySelectorAll(".error-message").length > 0; | |
| button.disabled = !isValid; | |
| } |
| errorMessage = document.createElement("span"); | ||
| errorMessage.className = "error-message"; | ||
| errorMessage.style.cssText = "color: red; font-size: 14px; display: block; margin-top: 4px;"; | ||
| errorMessage.innerText = "이메일을 입력해주세요."; | ||
| emailInput.closest(".input-group").appendChild(errorMessage); |
There was a problem hiding this comment.
💊 제안
JS 를 통해 스타일을 변경해주셔도 괜찮지만, error와 관련된 클래스를 만들어두시고 해당 클래스를 통해 스타일링하시면
코드도 줄고 css관련은 css 파일에 적으니 더 좋을 것 같네요!
| errorMessage = document.createElement("span"); | |
| errorMessage.className = "error-message"; | |
| errorMessage.style.cssText = "color: red; font-size: 14px; display: block; margin-top: 4px;"; | |
| errorMessage.innerText = "이메일을 입력해주세요."; | |
| emailInput.closest(".input-group").appendChild(errorMessage); | |
| errorMessage = document.createElement("span"); | |
| errorMessage.className = "error-message"; // css에서 .error-message { color: red; font-size: 14px; display: block; margin-top: 4px; } 와 같은 식으로 스타일링하세요~ | |
| errorMessage.innerText = "이메일을 입력해주세요."; | |
| emailInput.closest(".input-group").appendChild(errorMessage); |
There was a problem hiding this comment.
💊 제안
한 파일에서 최상단에 변수 선언들을 위치시키는 것처럼 일정한 순서로 코드가 작성되는 것이 가독성 측면에서 좋습니다~
지금 변수와 이벤트 바인딩문이 번갈아가며 작성되어 있고 그렇게 위치해야하는 이유도 잘 모르겠어요~
가능하면 이벤트 바인딩문은 변수 선언문보다 하단에 위치시키는 것을 추천드립니다!
| passwordInput.addEventListener("blur", validatePassword); | ||
| passwordInput.addEventListener("input", validatePassword); | ||
| } | ||
| function validatePassword() { |
There was a problem hiding this comment.
💊 제안
승민님이 너무 어려우셨다고 하셔서 기능적으로 크게 문제가 없는 부분은 피드백을 안 드리려고 하는데,
if문이 많이 중첩되면 로직 파악이 어려워집니다~
이러한 경우 함수로 많이 분리하면 흐름 따라가기가 더 쉬워집니다~
어려우시면 참고만해보세요.
function validateEmail() {
const email = emailInput.value.trim();
const errorMessage = getErrorMessageEl(
emailInput,
"이메일을 입력해주세요."
);
if (!email) {
showError(emailInput, errorMessage, "이메일을 입력해주세요.");
} else if (!isValidEmail(emailValue)) {
showError(emailInput, errorMessage, "잘못된 이메일 형식입니다");
} else {
hideError(emailInput, errorMessage);
}
checkFormValidity();
}
function isValidEmail(email) {
const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
return emailRegex.test(email);
}
function getErrorMessageEl(input, defaultMessage) {
const errorMessage =
input.closest(".input-group").querySelector(".error-message") ??
document.createElement("span");
errorMessage.className = "error-message";
errorMessage.innerText = defaultMessage;
input.closest(".input-group").appendChild(errorMessage);
return errorMessage;
}
function showError(input, errorMessage, message) {
input.classList.add("input-error");
errorMessage.innerText = message;
}
function hideError(input, errorMessage) {
input.classList.remove("input-error");
if (errorMessage) errorMessage.remove();
}| form.addEventListener("submit", function (e) { | ||
| e.preventDefault(); | ||
| if (button.disabled) return; | ||
| if (nicknameInput || passwordCheckInput) { | ||
| window.location.href = "login.html"; | ||
| } else { | ||
| window.location.href = "/items"; | ||
| } | ||
| }); |
There was a problem hiding this comment.
💊 제안
form이 유효하지 않으면 submit button이 disabled 상태라 제출이 되지 않으므로 관련 코드는 없어도 될 것 같아요~
| form.addEventListener("submit", function (e) { | |
| e.preventDefault(); | |
| if (button.disabled) return; | |
| if (nicknameInput || passwordCheckInput) { | |
| window.location.href = "login.html"; | |
| } else { | |
| window.location.href = "/items"; | |
| } | |
| }); | |
| form.addEventListener("submit", function (e) { | |
| e.preventDefault(); | |
| if (nicknameInput || passwordCheckInput) { | |
| window.location.href = "login.html"; | |
| } else { | |
| window.location.href = "/items"; | |
| } | |
| }); |
There was a problem hiding this comment.
💊 제안
한 파일에서 로그인, 회원가입 공용으로 JS를 작성해주셨네요~
둘이 공통되는 부분이 많이 이렇게 해주신 것 같아요.
다만 그렇게 되면 코드가 복잡해져서 유지보수성이 떨어지고 로그인 페이지의 경우 불필요한 코드까지 알아야합니다~
가능하면 공통 로직들은 분리해주시고 각 페이지별 JS를 따로 만드시는 것을 추천드려요!
우선은 간단하게 아래처럼 분리하셔도 좋을 것 같아요.
auth.js // 로그인, 회원가입에서 필요한 유효성 검사 로직들이 위치
login.js // auth.js에서 필요한 로직을 가지고와 필요한 태그에 연결
signup.js
1 participant
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
멘토에게