[박지섭] sprint4 #154
[박지섭] sprint4 #154
Conversation
- reset.css를 추가해서 구현 - 기존의 reset.css에서 구현한 코드를 base.css에서 관리 - home_adaptive.css 파일은 반응형으로 변경 전 코드를 이전(구현 후 제거 예정) - login.css, signup.css를 통합 구현하기 전 임시적으로 auth.css에서 관리
- 웹 접근성을 위해 시맨틱 태그를 변경하였습니다.
- 편의를 위해 1rem = 10px로 변경했습니다.
- 사소한 값은 자체 보정 했습니다.
- 비밀번호 보이게하는 버튼용 아이콘 에셋을 추가했습니다.
GANGYIKIM
left a comment
GANGYIKIM
left a comment
There was a problem hiding this comment.
지섭님 4번째 미션 제출 고생하셨습니다.
구현도 잘 하시고 정리도 잘하셔서 가독성이 좋은 코드였어요.
이제 React로 넘어가시는데 더 즐겁게 작업하실 것 같아요!
다음 미션도 화이팅입니다.
-
회원가입 페이지도 요구사항을 구현하셨다고 체크해주셨는데, 코드상에는 보이지 않네요! 확인해보시면 좋겠어요.
-
비밀번호 토글도 동작하지 않고, 코드상에서도 찾을 수가 없습니다.
-
script 파일들은 scripts 폴더 내에서 분리해서 구현했습니다. 파일 구조도 괜찮은지 확인 부탁드려요.:
지금 파일이 다 올라오지 않아서 의도를 파악하기 어려울 것 같습니만 지금 구조 기반으로 코멘트 드리겠습니다. 개인적으로 무난해보입니다만 login_validation.js, validation, validation/validation.js 등 중복되는 단어가 많이 들어가는 것 같습니다. 개인적으로 validation/validation.js 경우 폴더명과 파일명이 완전히 일치하니 다름 이름으로 변경하시는 것을 추천드리고, login_validation의 경우 login.js 정도면 충분하지 않을까합니다. -
formError를 createValidator에서 import를 해서 사용하는 구조로 구현했습니다. 최대한 의존도를 줄이고 싶었지만 그러면 분리를 하기 어려울 것 같아 지금처럼 구현했습니다. 잘못된 부분이 있으면 확인 부탁드려요.:
지금 코드상에서는 어떤 부분을 말씀하시는 지 확인하기 어려워 답변드리지 못할 것 같습니다.
| email_validate_regex: /^[^\s@]+@[^\s@]+\.[^\s@]+$/, | ||
| password_min_length: 8, | ||
| empty_message: '', | ||
| }); |
There was a problem hiding this comment.
💊 제안
SYMBOL이라는 객체명은 너무 많은 것을 의미해서 해당 값들의 의미를 잘 전달하지 못하는 것 같습니다.
해당 객체는 이메일 유효성 검사 정규식, 비밀번호 최소 길이 등을 포함하고 있으므로 이를 알 수 있는 이름으로 변경하시는 것을 추천드려요!
| </div> | ||
| </div> | ||
|
|
||
| <script type="module" src="./scripts/login_validation.js"></script> |
There was a problem hiding this comment.
💊 제안
script 태그는 상단에 있는게 구조 파악측면에서 유리하다고 생각해서 큰 이유가 없다면 상단 head 태그에 두시는 것을 추천드려요~
특히 타입이 모듈인 스크립트는 defer 속성을 자동으로 가지기 때문에 따로 하단에 배치하실 이유가 없습니다~
| export const EMAIL_VALIDATION = Object.freeze({ | ||
| invalid_email: '잘못된 이메일 형식입니다.', | ||
| required_email: '이메일을 입력해주세요.', | ||
| }); |
There was a problem hiding this comment.
💊 제안
EMAIL_VALIDATION 이름보다 EMAIL_ERROR_MESSAGE와 같이 에러문구 객체라는 것을 알 수 있는 이름이 더 좋을 것 같아요~
There was a problem hiding this comment.
👍 칭찬
확인할 수는 없지만 회원가입과 로그인에서 공용 로직을 같이 사용하기 위해 따로 빼주신 것 같아요!
중복을 줄이시고 주석도 자세히 달아두신 것 좋습니다.
| const emailError = validateEmail(emailInput.value.trim()); | ||
| const passwordError = validatePassword(passwordInput.value.trim()); | ||
|
|
||
| if (!emailError && !passwordError) { | ||
| window.location.href = '/items.html'; | ||
| } |
There was a problem hiding this comment.
💊 제안
폼의 값들이 유효하지 않으면 disabled 속성으로 인해 해당 이벤트가 발생하지 않으므로 여기서 유효성 검사를 할 필요가 없어보여요.
| const emailError = validateEmail(emailInput.value.trim()); | |
| const passwordError = validatePassword(passwordInput.value.trim()); | |
| if (!emailError && !passwordError) { | |
| window.location.href = '/items.html'; | |
| } | |
| window.location.href = '/items.html'; |
| const loginButton = document.querySelector('.auth__button'); | ||
| const loginForm = document.querySelector('form'); |
There was a problem hiding this comment.
💬 여담
제 생각에는 파일명을 통해 login 관련이라는 것을 알 수 있으므로 변수명에서 login을 빼도 될 것 같아요.
| const loginButton = document.querySelector('.auth__button'); | |
| const loginForm = document.querySelector('form'); | |
| const submitButton = document.querySelector('.auth__button'); | |
| const form = document.querySelector('form'); |
| const errorMessage = wrapper.querySelector('.auth__error-message'); | ||
| if (errorMessage) { | ||
| errorMessage.remove(); | ||
| } |
There was a problem hiding this comment.
💊 제안
해당 코드에서는 동적으로 태그를 삽입하면서 에러 메시지를 렌더링하는 방식입니다.
이렇게되면 계속적으로 DOM 조작을 해야하므로 error message가 들어갈 태그를 미리 만들어주고 에러에 따라 innerText만 변경하는 방식으로 수정하시는 것을 추천드려요.
| const validateEmailInput = () => { | ||
| const email = emailInput.value.trim(); | ||
| hideError(emailInput); | ||
|
|
||
| const error = validateEmail(email); | ||
| if (error) { | ||
| showError(emailInput, error); | ||
| } | ||
|
|
||
| updateButtonState(); | ||
| }; |
There was a problem hiding this comment.
💊 제안
해당 함수에 따르면 무조건 hideError 함수가 실행됩니다. showError 함수를 보면 에러메시지를 보여줄 태그가 존재할 경우 재활용을 하기 때문에 hideError도 필요시에만 호출하는 것이 성능과 가독성 측면에서도 좋을 것 같습니다.
| const validateEmailInput = () => { | |
| const email = emailInput.value.trim(); | |
| hideError(emailInput); | |
| const error = validateEmail(email); | |
| if (error) { | |
| showError(emailInput, error); | |
| } | |
| updateButtonState(); | |
| }; | |
| const validateEmailInput = () => { | |
| const error = validateEmail(emailInput.value.trim()); | |
| if (error) { | |
| showError(emailInput, error); | |
| } else { | |
| hideError(emailInput); | |
| } | |
| updateButtonState(); | |
| }; |
2 participants
🐼 판다마켓 - 미션4
🌐 미션 배포 주소: https://harryseop-pandamarket.netlify.app/
기본 요구사항
체크리스트 [기본]
로그인
회원가입
체크리스트 [심화]
변경 사항 [코드 리뷰]
미션1 ➡️ 미션2
!important키워드 제거미션2 ➡️ 미션3
미션3 ➡️ 미션4
주강사 님에게
script파일들은scripts폴더 내에서 분리해서 구현했습니다. 파일 구조도 괜찮은지 확인 부탁드려요.formError를createValidator에서 import를 해서 사용하는 구조로 구현했습니다. 최대한 의존도를 줄이고 싶었지만 그러면 분리를 하기 어려울 것 같아 지금처럼 구현했습니다. 잘못된 부분이 있으면 확인 부탁드려요.