-
Notifications
You must be signed in to change notification settings - Fork 39
[김서연] sprint4 #114
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 #114
The head ref may contain hidden characters: "Basic-\uAE40\uC11C\uC5F0-sprint4"
Conversation
…ve design in auth.css
… responsive design
… on the landing page and align FAQ text to the center of the footer on mobile
… already has a 16px margin
…submit background color gray to blue (버튼 활성화 jS 추가 예정)
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번째 PR 고생하셨습니다!
다음 React 미션도 화이팅입니다~
-
전체적인 기능은 구현했는데, 현재 코드에서 어떤 부분을 수정하는게 더 나은 방법인지 궁금합니다. (특히 비밀번호 눈 기능 구현한 부분은 조금 더 효율적으로 작성할 방법이 존재할 것 같은데, 확인 부탁드립니다!):
코멘트 드렸으니 확인해주세요~ 제가 남겨드린 코드는 서연님 현재 구조를 최대한 유지하며 작성한 것이니 다른 방식도 고민해보시면 더 재미있을거에요! -
자바스크립트 파일을 common, login, signup, password-visibility로 나누어서 관리를 하고 있는데, 보통 자바스크립트 파일은 어떤식으로 나누어 관리하는지 궁금합니다.:
보통 어떻게 관리하는지에 대해서는 제가 잘 몰라서 확실히 답변드리기가 어렵습니다만 대부분 코드의 중복을 줄이고 유지보수를 원할하게 하고자하는 목표를 가지고 파일을 관리합니다. 이러한 관점에서 CSS 파일도 분리를 하고 재사용을하는 겁니다. JS도 동일합니다. 중복은 줄이고 기준을 가지고 파일을 나눠, 분류를 해 가독성을 높이는 것이 좋습니다. -
이메일을 작성하고 나면 비밀번호에서 비밀 번호를 작성하라는 에러 메세지가 뜨는데, 원인이 무엇인지 모르겠습니다. (특히 회원 가입 페이지에서 보면 닉네임에는 에러 메세지가 뜨지 않는데, 비밀 번호에서는 비밀 번호를 작성하라는 에러가 뜹니다):
이는 login.js, signup.js의 validateButton 함수 때문입니다. 해당 함수 내부에서 if 조건문안에서 각 input이 valid 한지 확인을 하고 그 과정에서 에러메시지가 출력됩니다. 이메일이 그렇게 되는 이유는isValidEmail() && isValidPassword() && isValidNickName() && isValidPasswordConfrim()조건문에서 이메일이 가장 먼저 체크되고 false가 나면 나머지 로직이 실행되지 않기 때문입니다. -
태그 선택자를 가지고올 때 현재 저는 querySelector()로 다 불러왔는데 다른 방법들도 많은걸로 알고있습니다. 그냥 querySelector()만 사용해도 되는건지, 다른 방법은 어느 경우에 사용되는지 궁금합니다.:
getElementById, classname등 여러가지 방법이 있습니다만 어떤 것을 쓰시던 크게 상관이 없습니다. mdn에서 궁금한 메소드에 대해 찾아보시면 도움이 되실거에요! -
현재 코드에서는 이벤트 리스너가 각 태그마다 적용되고 있는데, 적용해야 할 이벤트 리스너가 많아진다면 태그에 하나하나 적용하는것이 비효율적일 거라 생각이 드는데(같은 이벤트에 같은 함수를 사용한다면 더욱 더), 이 부분에 대해 조금 더 효율적으로 이벤트 리스너를 활용할 수 있는 방법이 있는지 궁금합니다.:
추후 배우실거지만 이벤트 전파방식에 따라 더 효율적으로 작성이 가능합니다. 이벤트 버블링을 검색해보시면 자세한 내용이 나옵니다~ 찾아보시고 적용해보시면 좋겠네요! (만약 이벤트 바인딩을 하시는 것이 싫으시다면 함수를 내보내서 html에서 onclick, onsubmit 에 연결해주는 방식도 있습니다) -
자바스크립트로 html까지 다루다보니 각 태그별로 class와 id가 복잡해지는 것 같습니다. 현재 코드에서 class와 id를 적절히 잘 사용한건지 확인 부탁드립니다! (+ 자바스크립트로 html을 다룰 때 언제 class를 가져오는게 좋고, 언제 id를 가져오는게 좋은건가요?):
어떤 의미에서 이러한 말씀을 해주신지 잘 모르겠습니다~ 개인적으로 id는 정말 고유한 값이 필요할때가 아니면 사용하지 않는 것이 좋다고 생각합니다. 자바스크립트로 html 컨트롤할때 id 값이 있는 요소라면 getElementById 같은 선택자를 통해 요소를 가지고 오는게 조금 더 빠르다고 합니다.
| <link rel="stylesheet" href="/auth/auth.css" /> | ||
|
|
||
| <!-- script 불러오기 --> | ||
| <script defer type="module" src="/auth/common.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.
❗️ 수정요청
module js의 경우 자동으로 defer 되기 때문에 defer 속성을 빼셔도 됩니다~
| <script defer type="module" src="/auth/common.js"></script> | |
| <script type="module" src="/auth/common.js"></script> |
| button { | ||
| cursor: pointer; | ||
| background-color: transparent; |
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.
💊 제안
버튼의 cursor pointer 속성은 일반적으로 공통으로 필요한 속성이라 common에 들어가기 적절하다고 생각했는데 빠지니 아쉽네요~
background-color 속성의 경우 reset과 겹치고 브라우저 스타일을 초기화하는 코드니 reset에서 작성하는 것이 더 적절해보입니다~
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.
💊 제안
normalize와 하는 일이 비슷해보이니 normalize import를 삭제하시고 브라우저 관련 초기화 코드를 해당 파일에 추가하시는 것이 더 좋아보여요~
지금은 reset과 normalize로 같은 역할을 하는 친구들이 나눠진 느낌입니다~
| button.disabled = true; | ||
| button.style.cursor = "not-allowed"; |
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.
💊 제안
로그인, 회원가입 페이지에서 기본적으로 disabled 속성을 가지고 있기 때문에 해당 부분이 없어도 될 것 같아요!
버튼이 비활성화 되었을 때 cursor를 not-allowed로 해주신 것은 좋지만 이는 css로도 가능한 부분이라 css로 처리하시는 것이 더 좋을 것 같아요~
| button.disabled = true; | |
| button.style.cursor = "not-allowed"; |
// common 이나 button 관련 css 파일
button:disabled {
cursor: not-allowed
}| // email 유효성 검사 패턴 | ||
| const pattern = /^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$/; |
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에서 모두 사용되니 따로 분리하셔서 같이 써도 될 것 같아요~
| return false; | ||
| } else if (!pattern.test(emailValue)) { // 정규식으로 이메일 형식 검사하기 | ||
| showError(inputEmail, emailError, "잘못된 이메일 형식입니다."); | ||
| return false; | ||
| } else { | ||
| hiddenError(inputEmail, emailError); | ||
| return 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.
❗️ 수정요청
early return 을 하고 계시기 때문에 else if문으로 사용하지 않고 아래처럼 작성하시는 것이 동일동작하며 가독성측면에서도 더 유리합니다~
| return false; | |
| } else if (!pattern.test(emailValue)) { // 정규식으로 이메일 형식 검사하기 | |
| showError(inputEmail, emailError, "잘못된 이메일 형식입니다."); | |
| return false; | |
| } else { | |
| hiddenError(inputEmail, emailError); | |
| return true; | |
| } | |
| return false; | |
| } | |
| if (!pattern.test(emailValue)) { | |
| showError(inputEmail, emailError, "잘못된 이메일 형식입니다."); | |
| return false; | |
| } | |
| hiddenError(inputEmail, emailError); | |
| return true; |
| const validateButton = () => { | ||
| if (isValidEmail() && isValidPassword()) { | ||
| button.disabled = false; | ||
| button.style.cursor = "pointer"; |
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.
❗️ 수정요청
버튼의 disabled 속성만 js로 조작하시고 css cursor 속성은 css를 통해 조작하시는 것을 추천드려요~
위에서 말씀드린 것처럼 disabled시 js에서 cursor속성을 not-allowed 로 바꾸는 코드를 제거하면 해당 코드도 불필요해집니다!
| button.style.cursor = "pointer"; |
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.
💊 제안
비밀번호 토글하는 함수 isPwVisibility와 비밀번호 확인 토글하는 함수 isPwConfirmVisibility가 겹치는 부분이 많아 보여요~
최대한 서연님의 코드를 유지하면서 중복을 줄이는 예시입니다~ 아래처럼 중복을 줄일 수 있게 작성하시는 것을 추천드려요!
(다른 부분도 중복을 줄이는 방법을 고민해보시면 더 좋습니다! )
const pwVisibility = document.querySelector(".pw-visibility");
const eyeImage = document.querySelector(".eye_icon");
const pwConfirmVisibility = document.querySelector("#password-confirm");
const pwConfirmEyeImage = document.querySelector("#password-confirm-eye");
const togglePassword = () => {
if (input.type === "password") {
// password면 text로
img.src = "/image/password_show_icon.svg";
input.type = "text";
} else {
// text면 password로
img.src = "/image/password_toggle_icon.svg";
input.type = "password";
}
};
const isPwVisibility = () => togglePassword(pwVisibility, eyeImage);
const isPwConfirmVisibility = () =>
togglePassword(pwConfirmVisibility, pwConfirmEyeImage);
eyeImage.addEventListener("click", isPwVisibility);
pwConfirmEyeImage.addEventListener("click", isPwConfirmVisibility);
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
멘토에게