Conversation
로그인,회원가입 validation 추가 및 반응형 디자인 추가
Sprint 1,2 origin 반영
validation.js에서 불필요한 코드를 제거하고 스타일 일부를 정리하였습니다.
| function createOrUpdateMetaTag(name, content, isProperty = false) { | ||
| // 표준 메타태그인지 Open Graph 메타태그인지 구별하기 위해 Property 여부를 확인 | ||
| let metadata = document.querySelector(`meta[${isProperty ? 'property' : 'name'}="${name}"]`); | ||
| if (!metadata) { | ||
| metadata = document.createElement('meta'); | ||
| if (isProperty) { | ||
| // Open Graph 메타태그는 property 속성을 사용 | ||
| metadata.setAttribute('property', name); | ||
| } else { | ||
| // 표준 메타태그는 name 속성을 사용 | ||
| metadata.name = name; | ||
| } | ||
| document.head.appendChild(metadata); | ||
| } | ||
| // 메타태그의 content 속성을 설정 | ||
| metadata.content = content; | ||
| } |
There was a problem hiding this comment.
true로 동작을 제어하는 플래그 방식의 함수가 정의되었다면, 이 경우는 아예 함수를 분리하는 게 좋을 거라는 신호일 수 있습니다.
만약 현재 상황에서 SEO를 위해 twitter cards나 schema.org 요소를 추가해야 하는 상황이 된다면 어떨까요?
| main > #logo, | ||
| main > .auth-form, | ||
| main > .social-login, | ||
| main > .auth-prompt { | ||
| flex: 0 1 40rem; | ||
| } |
There was a problem hiding this comment.
우선 id selector로 접근하는 #logo의 경우는 굳이 main > 같은 표현을 사용할 필요는 없을 것 같네요. 나머지 클래스에 리스트 형태로 스타일을 지정하는 방식도 나쁘진 않지만, auth-form, social-login, auth-prompt가 동일한 기능을 하고 있는 부분이 있다면 해당 영역만 content-wrapper 같은 식의 재사용할 수 있는 클래스로 정의하는 것도 괜찮은 접근일 것 같습니다.
scripts/validation.js
Outdated
| signupForm?.addEventListener('submit', handleSignupSubmit); | ||
| loginForm?.addEventListener('submit', handleLoginSubmit); | ||
| emailInput.addEventListener('focusout', validateEmail); | ||
| // 로그인 페이지에는 닉네임 입력이 없으므로 Optional Chaining 사용 | ||
| nicknameInput?.addEventListener('focusout', validateNickname); | ||
| passwordInput.addEventListener('focusout', validatePasswords); | ||
| // 비밀번호 확인 입력 시 비밀번호 validation | ||
| confirmInput?.addEventListener('input', validatePasswords); | ||
| pwToggleButtons.forEach(button => { | ||
| button.addEventListener('click', () => togglePassword(containers)); | ||
| }); |
There was a problem hiding this comment.
element에 이벤트를 할당할 때 optional chaining으로 처리하고 있는 이유가 element는 그 자체로써 HTML에 종속적인 구조를 가지고 있기 때문이죠.
그렇다면 이런 케이스는 모든 로직을 js 파일 내에서 처리하려고 하기보다는 element를 쿼리하는 부분은 제어할 수 없음을 인정하고 js 영역에서는 이벤트를 할당할 수 있는 addFormEvent 등의 함수를 정의해두고 HTML 파일 내 script 영역에서 쿼리한 element에 이벤트를 할당하는 것도 고려해볼 수 있을 것 같습니다.
사실 더 나아가면 이벤트 위임 패턴이나 작성하신 것과 비슷한 패턴으로 스크립트 내에서 이벤트를 핸들링하는 식으로 처리할 수도 있습니다. 물론 이것은 충분히 일반화가 진행되어야 한다는 전제가 따라붙지만요. 지금으로썬 단계별로 잘 개선되고 있는 것 같네요.
scripts/validation.js
Outdated
| setTimeout(() => { | ||
| toast.classList.remove('hide'); | ||
| toast.classList.add('show'); | ||
| }, 10); | ||
| setTimeout(() => { | ||
| fadeOut(toast); | ||
| isToastShow = false; | ||
| }, 1000); |
There was a problem hiding this comment.
- 입력한 값이 검증 규칙에 어긋났을 때 실패 메시지를 보여주고 fadeOut시키는 게 좋은 패턴일까요? 🤔
- 그리고 간단한 애니메이션의 경우는 CSS의 animation을 사용하는 게 JS로 직접 처리하는 것보다는 여러모로 이롭습니다. (CSS와 JS의 분리라거나, 성능면에서라거나)
scripts/validation.js
Outdated
| function isPasswordValid(password) { | ||
| return password && password.length >= 8; | ||
| } | ||
|
|
||
| function validatePasswords(event) { | ||
| event.preventDefault(); | ||
| // 비밀번호 확인 input이 존재하는지 확인 | ||
| const isConfirmInputExist = !!confirmInput; | ||
|
|
||
| // 비밀번호와 비밀번호 확인 input이 모두 존재하고 두 값이 일치하지 않는 경우, 둘 다 data-invalid 속성 추가 | ||
| const isValid = !isConfirmInputExist || passwordInput.value === confirmInput.value; | ||
| const hasValue = passwordInput.value && (!isConfirmInputExist || confirmInput.value); | ||
|
|
||
| passwordInput.setAttribute('data-invalid', hasValue && !isValid); | ||
| confirmInput?.setAttribute('data-invalid', hasValue && !isValid); | ||
|
|
||
| if (!hasValue) { | ||
| passwordInput.removeAttribute('data-invalid'); | ||
| confirmInput?.removeAttribute('data-invalid'); | ||
| } | ||
|
|
||
| // 비밀번호 확인 input이 존재하지 않는 경우, 비밀번호만 검증 | ||
| const validations = [ | ||
| { | ||
| condition: !passwordInput.value, | ||
| input: passwordInput, | ||
| message: validityMessage.passwordIsEmpty, | ||
| }, | ||
| { | ||
| condition: !isPasswordValid(passwordInput.value), | ||
| input: passwordInput, | ||
| message: validityMessage.passwordIsTooShort, | ||
| }, | ||
| { | ||
| condition: isConfirmInputExist && !confirmInput.value, | ||
| input: confirmInput, | ||
| message: validityMessage.passwordIsEmpty, | ||
| }, | ||
| { | ||
| condition: isConfirmInputExist && !isPasswordValid(confirmInput.value), | ||
| input: confirmInput, | ||
| message: validityMessage.passwordIsTooShort, | ||
| }, | ||
| { | ||
| condition: isConfirmInputExist && passwordInput.value !== confirmInput.value, | ||
| input: confirmInput, | ||
| message: validityMessage.passwordIsNotMatch, | ||
| }, | ||
| ]; | ||
|
|
||
| // 조건, 입력값, 메세지 순서대로 순회하며 조건이 참일 경우 메세지를 띄우기 | ||
| for (const { condition, input, message } of validations) { | ||
| if (condition) { | ||
| showToast(input, message); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
패스워드를 검증하기 위해 많이 고민한 흔적이 드러나는 것처럼 보이네요. 하지만 몇 가지 개선할 수 있을만한 지점들이 보이는 것 같네요.
이 부분은 같이 한번 뜯어보면 좋을 것 같아서 다음 멘토링 시간에 한번 봅시다.
There was a problem hiding this comment.
안녕하세요, 현우님.
이전 미션에서 리뷰를 남겼던 부분을 반영해주셨군요! 수고하셨습니다.
노파심(?)에 한 말씀 드리자면, 제가 남기는 리뷰도 어디까지나 개인의 의견이기 때문에 제가 남긴 작성한 리뷰를 모두 반영한다는 느낌보다는 제가 미처 제대로 파악하지 못한 부분이 있을 수도 있으니 비판적으로 검토한 후에 아니라고 생각되는 부분이 있다면 추가로 논의를 진행한 후에 결정하는 것도 충분히 가능한 사안입니다.
그리고 다음부터는 커밋 단위도 신경 쓰면서 잘 나눠보시면 더 좋을 것 같습니다.
데이터와 액션과 연산을 용도에 맞게 모듈화하였습니다.
…n-fe into basic-이현우-sprint3
리뷰 감사합니다. 안그래도 다른 분들 제출한것을 보고 커밋을 좀 더 잘게 쪼개서 해야 하는데 git의 장점을 제대로 활용하지 못하고 있다는 생각이 들었습니다. 다음 미션부터는 이를 명심해서 커밋해 보겠습니다. 조언 감사합니다. |
주요 변경사항
배포 결과
이전 코드리뷰 반영
기본 요구사항
로그인, 회원가입 페이지 공통
로그인 - 팝업
회원가입 - 팝업
심화 요구사항
전체(로그인, 회원가입, 랜딩 페이지) 공통
로그인, 회원가입 페이지 공통
로그인 - 팝업
회원가입 - 팝업
랜딩 페이지