-
Notifications
You must be signed in to change notification settings - Fork 39
[김정우] Sprint4 #169
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 #169
The head ref may contain hidden characters: "Basic-\uAE40\uC815\uC6B0-sprint1"
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번째 미션 작업 수고하셨습니다!
GPT를 활용하는 것도 좋고 이해하시면서 작업하셨다니 더 좋네요.
다음 React 미션도 화이팅입니다!
자바스크립트 파일을 한번에 관리할 수 있을 거 같아 같은 파일에 작성해보려고 시도했는데, 오류가 떠서 분리해서 작성했습니다. 합쳐서 관리하는 게 좋은지, 따로 분리하는 게 좋을지 궁금합니다.:
로그인과 회원가입에서 겹치는 스타일이 많아 auth.css 하나로 작성하신 것처럼 겹치는 것은 줄이시는 것이 좋습니다. 이렇게 중복을 줄이는 다양한 방법이 있는데, 지금의 경우 각 login.js, signup.js에서 공통으로 사용하는 로직들을 다른 파일로 분리하시고, 이를 login.js, signup.js 파일에서 사용하시는 방법이 좋습니다.
아래 코드는 설명을 위한 코드로 참고만해주세요~
// 로그인, 회원가입 페이지에서 다 사용하는 이메일 형식 확인 함수
const checkIsValidEmail = (email) => { ... }
export { checkIsValidEmail }// login.js
import { checkIsVaildEmail } from './'
document.addEventListener("DOMContentLoaded", () => {
const emailInput = document.getElementById("userName");
emailInput.addEventListener("input", () => {
if (checkIsVaildEmail(emailInput.value)) {
emailInput.classList.remove("error");
errorMessage.textContent = "";
errorMessage.style.display = "none";
}
updateButtonState();
});
// signup.js
import { checkIsVaildEmail } from './'
document.addEventListener("DOMContentLoaded", () => {
const emailInput = document.getElementById("userName");
emailInput.addEventListener("input", () => {
if (checkIsVaildEmail(emailInput.value)) {
emailInput.classList.remove("error");
errorMessage.textContent = "";
errorMessage.style.display = "none";
}
updateButtonState();
});| <a class="membership" href="signup.html">회원가입</a> | ||
| </div> | ||
| </div> | ||
| <script src="login.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 속성을 추가하시면 되겠습니다~
| <input | ||
| type="text" | ||
| name="userName" | ||
| name="nicName" |
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.
❗️ 수정요청
오타이신 것 같아요~
| name="nicName" | |
| name="nickname" |
| const errorMessage = document.getElementById("error-message"); | ||
| const errorPassword = document.getElementById("error-password"); |
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.
💊 제안
이름은 코드에 있어 내용을 파악하는데 도움을 주는 중요한 단서입니다.
지금 errorMessage의 경우는 이메일관련 에러를 나타내는 태그의 클래스명인데 일반 명사인 errorMessage로 이름이 지어져 있어
errorPassword와 비교해서 어떤 변수인지 명확하지 않습니다.
가능하면 어떤 변수인지 잘 설명해줄 수 있는 이름으로 변경해주세요~
| const errorMessage = document.getElementById("error-message"); | |
| const errorPassword = document.getElementById("error-password"); | |
| const errorEmail = document.getElementById("error-message"); | |
| const errorPassword = document.getElementById("error-password"); |
| emailInput.addEventListener("input", () => { | ||
| if (emailInput.value !== "" && emailInput.value.includes("@")) { |
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 input의 값이 비어있는지 확인하게 되면 " " 와 같이 공백만 존재하는 경우에도 값이 있다고 판단하게 됩니다.
따라서 string의 trim method를 통해 공백을 제거하시면 의도한 바와 더 가까울 것 같습니다.
조건문에서 이메일 형식을 확인하는 emailInput.value.includes("@") 부분의 경우도 @ 라는 요소가 들어가있는지만 확인하는 것이라 명확한 이메일 형식 확인은 되지 않고 있으니 이메일 관련 정규식을 통해 구현하시면 더 좋겠습니다~
| emailInput.addEventListener("input", () => { | |
| if (emailInput.value !== "" && emailInput.value.includes("@")) { | |
| emailInput.addEventListener("input", () => { | |
| if (emailInput.value.trim() !== "" && emailInput.value.includes("@")) { |
| loginBtn.addEventListener("click", function (e) { | ||
| e.preventDefault(); | ||
| if (!loginBtn.disabled) { | ||
| window.location.href = "/item"; | ||
| } | ||
| }); |
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.
💊 제안
form이 유효하지 않으면 submit button이 disabled 상태라 제출이 되지 않으므로 관련 코드는 없어도 될 것 같아요~
| loginBtn.addEventListener("click", function (e) { | |
| e.preventDefault(); | |
| if (!loginBtn.disabled) { | |
| window.location.href = "/item"; | |
| } | |
| }); | |
| loginBtn.addEventListener("click", function (e) { | |
| e.preventDefault(); | |
| window.location.href = "/item"; | |
| }); |
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.
💊 제안
form이 유효하게 작성되어 있는지를 판단해 버튼을 활성화하는 updateButtonState 함수를 input 이벤트와 focusout 이벤트 시 모두 실행하고 있네요. 다만 input 이벤트에서 해당 함수를 호출한다면 이미 값이 변경되는 시점에 updateButtonState 함수를 호출했기 때문에 focusout시에는 호출하지 않아도 될 것 같습니다.
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게