[김동한] Sprint4 #145
[김동한] Sprint4 #145
Conversation
GANGYIKIM
left a comment
GANGYIKIM
left a comment
There was a problem hiding this comment.
동한님 4번째 미션 제출 고생하셨습니다.
다음 미션도 화이팅입니다~
-
요구 사항이 잘 구현되었는지 궁금합니다.:
대부분 잘 구현되었다고 생각됩니다. 다만 동작 측면에서 어색한 부분이 있어 코멘트 드렸으니 확인해보세요. -
코드에서 메소드와 변수들이 적절이 선언되고 사용되었는지 궁금합니다.:
이는 개인의 기준에 따라 다를 것 같습니다. 취향의 영역에 있어서는 크게 코멘트 드리지 않고 각자의 스타일에 따라 작성하는 것이 좋다고 생각합니다~ 다만 중복되는 코드는 줄이는 것이 좋다고 생각해 코멘트 드렸으니 참고해주세요. -
가독성을 개선시키고 싶은데 팁이 있으신지 궁금합니다.:
가독성을 위한 중요한 요소는 적절한 이름과 적절한 분리입니다. 한 함수가 하나의 역할을 하게 작성하고 이에 적절한 이름을 붙이는 연습을 하시는 것이 좋습니다. 또한 내가 처음 이 코드를 보는 개발자라고 생각하고 이해하기 어려운 부분이 없는지 생각해보시는 것도 좋습니다. -
변수 네이밍을 하는데 어려움이 있었는데 적절히 네이밍 되었는지 궁금합니다.:
네이밍도 취향의 영역에 들어간다고 생각해 크게 피드백 드리지 않았습니다만 해당 값을 유추할 수 있는 이름이라면 괜찮다고 생각합니다. -
코드에서 불필요한 부분이 있거나 좀 더 효율성 있게 코드를 만들 수 있는 부분이 있는지 궁금합니다.:
코멘트 참고해주세요~
| <button class="login_button btn_disable auth_button" type="submit"> | ||
| <a href="/items.html">로그인</a> | ||
| </button> |
There was a problem hiding this comment.
❗️ 수정요청
a 태그와 button 태그는 중첩해서 사용하지 않습니다~
저희 요구사항의 경우 input들의 값이 유효하면 버튼이 눌리고 그 다음 페이지 이동이니
button 태그만 남겨두시고 js 를 통해 페이지를 이동시켜주세요~
| </div> | ||
| <p>판다마켓이 처음이신가요? <a href="/signup.html" class="signup_link">회원가입</a></p> | ||
| </main> | ||
| <script src="/login.js"></script> |
There was a problem hiding this comment.
💊 제안
아마 script 태그를 HTML의 하단에 배치하신 이유가 script가 문서의 렌더링을 막지 않도록 하기 위해서이실 것 같아요.
하지만, script 태그에 defer나 async 속성을 사용하면 이런 문제를 해결할 수 있기 때문에 반드시 하단에 배치할 필요는 없습니다!
또한 script 태그는 상단에 있는게 구조 파악에서도 유리하기 때문에 상단 head 태그에 두시는 것을 추천드려요~
지금과 같은 경우 DOM을 조작하는 JS 이니 defer 속성을 추가하시면 되겠습니다~
| @@ -0,0 +1,100 @@ | |||
| const EMAIL_PATTERN = /^[A-Za-z0-9_\.\-]+@[A-Za-z0-9\-]+\.[A-za-z0-9\-]+/; | |||
There was a problem hiding this comment.
💊 제안
signup 파일에서도 동일한 정규식을 사용하고 있는데 그때마다 선언해서 쓰면 문제가 있을 것 같아요.
정규식을 반복해서 선언하면 유지보수가 어려워지고, 코드의 일관성이 떨어질 수 있습니다. 만약 정규식을 수정해야 할 때 여러 곳에서 변경해야 하고 실수로 서로 다른 정규식을 사용할 위험도 커집니다.
이렇게 두개의 파일에서 사용하고 있으니 해당 정규식과 test 함수를 따른 파일로 분리해 사용하시는 것을 추천드려요!
| } | ||
| } | ||
|
|
||
| function showAlert(inputField, element, message) { |
There was a problem hiding this comment.
❗️ 수정요청
해당 함수는 에러 메시지를 보여주는 함수네요. alert 라는 브라우저 메소드가 있으니 헷갈리지 않고 더 명확하게 showErrorMessage 같은 이름을 쓰실 것을 권합니다.
| function inputInit() { | ||
| inputAll.forEach((input) => { | ||
| input.value = ''; | ||
| }); | ||
| } |
There was a problem hiding this comment.
❗️ 수정요청
모든 인풋의 값을 비워주는 함수가 왜 필요한지 모르겠어요~
로그인 버튼을 눌렀을 때는 저희 요구사항 활성화된 '로그인' 버튼을 누르면 "/items" 로 이동합니다 에 따라 페이지로 이동시켜주면 될 것 같습니다~
| authBtnLink.addEventListener('click', (e) => authButtonActivate(e)); | ||
| authButton.addEventListener('click', inputInit); | ||
| passwordVisible.addEventListener('click', visibleBtnHandler); | ||
| inputAll.forEach((tag) => { |
There was a problem hiding this comment.
💊 제안
인자명은 input이 더 가독성 측면에서 좋을 것 같아요~
| inputAll.forEach((tag) => { | |
| inputAll.forEach((input) => { |
There was a problem hiding this comment.
💊 제안
로그인에서 사용하는 js 파일과 에러메시지나 로직 측면에서 동일한 것이 많습니다~
이런 경우 중복을 줄이기 위해 정규식, 에러메시지들은 다른 파일로 분리하시고 각 js 파일에서 가져와서 사용하시는 것을 추천드려요!
There was a problem hiding this comment.
❗️ 수정요청
로그인 버튼 활성화에 대해 테스트해보시면 좋겠습니다.
제가 테스트해본 경우 비밀번호가 유효하지 않은 상태에서 포커스아웃하고 다시 비밀번호를 유효하게 입력해도
버튼이 비활성화 상태로 존재하네요!
의도한 동작이 아니라면 수정하시면 좋겠습니다~
| checkPassword = e.target.value; | ||
| pwInputStatus = true; | ||
| } | ||
| } | ||
|
|
||
| function passwordDoubleCheck(e) { | ||
| if (e.target.value !== checkPassword) { |
There was a problem hiding this comment.
💊 제안
이렇게되면 비밀번호가 유효하게 입력된 경우에만 비밀번호 확인에서 해당 비밀번호와 비교를 할 수 있게 되고
비밀번호가 유효하지 않은 상태에서 비밀번호 확인을 입력하면 부정확한 에러메시지가 뜹니다.
예를 들어 password: 1234, passwordConfirm: 1234의 경우에도 비밀번호는 일치하지만 '비밀번호가 일치하지 않습니다.' 에러가 나오게 됩니다~
의도한 동작이 아니라면 확인 후 수정해보세요!
2 participants
요구사항
기본
####로그인
####회원가입
심화
주요 변경사항
스크린샷
멘토에게