-
Notifications
You must be signed in to change notification settings - Fork 37
[이준영] sprint4 #153
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 #153
The head ref may contain hidden characters: "Basic-\uC774\uC900\uC601-sprint4"
Conversation
Lanace
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.
너무 깔끔하게 잘 작성해주신것같습니다...!
고생 많으셨어요ㅋㅋ!
|
|
||
| .pw-eye-btn-on, | ||
| .pw-eye-btn-off, | ||
| .pw-ck-eye-btn-on, | ||
| .pw-ck-eye-btn-off { | ||
| .pw-eye-btn, | ||
| .pw-ck-eye-btn { | ||
| position: absolute; | ||
| top: 16px; | ||
| right: 16px; | ||
| } |
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.
오... 이건 좀 놀라긴 했네요...!
삭제된거까지 같이 없애주는거 정말 좋은 습관이에요!
더이상 사용하지 않는 스타일 코드는 삭제해주시는게 좋거든요
놓치기 쉬운건데 꼼꼼하게 잘 챙겨주셨네요ㅎㅎ
| .error-message { | ||
| padding-left: 16px; | ||
| margin-top: 8px; | ||
| font-weight: 600; | ||
| font-size: 14px; | ||
| line-height: 24px; | ||
| color: var(--red); | ||
| display: none; | ||
| } |
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.
error-message 의 display 기본값을 none으로 하기엔 아래 .hide 스타일로 처리할 수 있을것같아서
저라면 error-message는 display를 초기값으로 그대로 두고, hide를 넣었다 뺐다 해서 화면에 노출시킬것같아요ㅎㅎ
| if (e.target.getAttribute('src') === '/images/login/pw-eye-btn-off.png') { | ||
| e.target.setAttribute('src', '/images/login/pw-eye-btn-on.png'); | ||
| pwInput.setAttribute('type', 'text'); | ||
| } else { | ||
| e.target.setAttribute('src', '/images/login/pw-eye-btn-off.png'); | ||
| pwInput.setAttribute('type', '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.
물론 src 에 있는 값을 통해 현재 상태를 구분해서 처리할 수 있어요ㅎㅎ!
틀린코드는 아닌데, 관리하기가 어려울것같아요.
ex) 이미지 아이콘이 변경된경우나 이미지 파일명이 변경된 경우에 까먹지 않고 auth.js 파일에 와서 변경된 부분을 같이 수정해주어야 하거든요.
만약
if (pwInput.type === "text") {
e.target.setAttribute('src', '/images/login/pw-eye-btn-on.png');
pwInput.setAttribute('type', 'text');
} else {
e.target.setAttribute('src', '/images/login/pw-eye-btn-off.png');
pwInput.setAttribute('type', 'password');
}이렇게 해주면 굳이 이미지 경로를 알 필요까진 없을거에요ㅎㅎ!
(물론 지금도 잘 동작합니)
| } | ||
|
|
||
| // auth 버튼 비활성화/활성화 상태 변경 | ||
| function authBtnStatusChange() { |
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.
authBtnStatusChange 를 위쪽에서 사용하는데 authBtnStatusChange 함수는 아래에 정의가 되어있어요.
컴퓨터는 코드를 위에서부터 아래로 코드를 읽는데 authBtnStatusChange 가 정의되지 않은 상태에서도 authBtnStatusChange 함수를 찾아갔어요...!
이런 현상을 호이스팅 이라고 하는데, 한번 이번기회에 찾아보시면 좋을것같아요ㅎㅎ!
만약 function () 으로 함수를 생성하지 않고 const = () => {} 로 생성하셨으면 문제가 생겼을 수 있어요ㅠ
| // 상태 초기화 함수(에러 메세지를 숨기고 입력 필드의 테두리를 원상태로 초기화) | ||
| function hideError(input, errorId) { | ||
| const errorElement = document.getElementById(errorId); | ||
| errorElement.style.display = 'none'; | ||
| input.style.border = 'none'; | ||
| } |
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.
와 이거 너무 잘하셨는데여ㅋㅋㅋ
공통적으로 반복되는 부분들을 이런식으로 뺴고 재사용을 많이 합니다...!
| function hideError(input, errorId) { | ||
| const errorElement = document.getElementById(errorId); | ||
| errorElement.style.display = 'none'; | ||
| input.style.border = 'none'; |
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.
조금더 개선한다고 하면 이러한 부분들을 직접 스타일 조정하기보단 class로 처리해주시는게 좀더 좋긴 해요...!
inline style이 그다지 좋진 않거든요ㅠ
나중에 규모가 커지면 어디서 어떻게 바뀐건지 추적하기가 어려워져요ㅠ
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.
네이밍도 좀더 좋은것같네요ㅎㅎ
| <span id="emailEmptyError" class="error-message"> | ||
| 이메일을 입력해주세요. | ||
| </span> | ||
| <span id="emailInvalidError" class="error-message"> | ||
| 잘못된 이메일 형식입니다. | ||
| </span> |
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.
이렇게 각각의 에러 상황을 표시해주는것도 좋지만 하나만 만들어두고 innterText를 바꾸는 식으로 접근하는건 어떨까요?
왜냐하면 이렇게 되어있으면 실수로 동시에 2개의 에러 메시지가 나올 가능성도 있기도 하고, DOM이 조금 지저분해질 수 도 있어서요ㅠ
물론 지금은 규모가 작기도 해서 괜찮지만요ㅎㅎ
요구사항
기본
로그인
회원가입
심화
주요 변경사항
멘토에게