-
Notifications
You must be signed in to change notification settings - Fork 42
[홍성현] sprint4 #145
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
The head ref may contain hidden characters: "Basic-\uD64D\uC131\uD604-sprint4"
[홍성현] sprint4 #145
Conversation
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.
수고하셨습니다!
전체적으로 코드도 깔끔하게 읽히고 좋은 시도도 많았네요 :)
제가 드린 코멘트도 보시고 리팩토링해보시면 더 좋은 결과물 만들수있을것같아요.
주요 리뷰 포인트
- 유지보수를 고려한 개발
@@ -89,12 +92,21 @@ <h2 class="feature-section__headline">인기 상품을 | |||
class="feature-section__content feature-section__content--reverse" | |||
> | |||
<h3 class="feature-section__title">Search</h3> | |||
<<<<<<< HEAD |
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.
여기 충돌해결하면서 생긴 라인들 그대로 남아있네요! 정리해주세요 :)
values: [emailInput.value, passwordInput.value], | ||
buttonEl: loginButton, | ||
}); | ||
} |
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.
20번과 21번째 사이 한칸 띄워주세요 :)
buttonEl.classList.toggle("active", isValid); | ||
} | ||
|
||
export function checkFormValidity({ errors, values, buttonEl }) { |
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.
UI 관련해서도 이런 재사용 가능한 공통 로직을 분리하신점 아주 좋은 시도입니다 👍 굳굳!
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.
다만 checkFormValidity()
의 인자를 줄일 수는 없을까? 하는 생각은 들어요.
지금 UI 구조상으로 값이 비어있든 유효성 검사가 통과되지않았든 결국엔 똑같은 방법으로 UI를 업데이트해주고있으니, validation을 체크하면서 유효성 검사 결과값과 메시지를 리턴하는 구조를 채택한다면 errors와 empty를 구분할 필요도 없어지고 인자도 필요없어지지않을까요? :)
@@ -0,0 +1,35 @@ | |||
import { showError, clearError, isValidEmail } from "./validation.js"; | |||
|
|||
export function validateEmail(inputEl, errorEl) { |
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.
validateEmail(), validatePassword() 내부를 보면 비슷한 로직이 반복되고있어요.
이런 객체를 만들어 관리해주면 어떨까요?
예시)
export const validators = {
email: (value) => {
if (!value) return { isValid: false, message: "이메일을 입력해주세요" };
if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value)) {
return { isValid: false, message: "올바른 이메일 형식이 아닙니다" };
}
return { isValid: true, message: "" };
},
password: (value) => {
if (!value) return { isValid: false, message: "비밀번호를 입력해주세요" };
if (value.length < 8) {
return { isValid: false, message: "비밀번호는 8자 이상이어야 합니다" };
}
return { isValid: true, message: "" };
},
};
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.
그리고 유효성 검사를 수행하는 함수에서 이런식으로 써준다면
if (result.isValid) {
clearError(inputEl);
} else {
showError(inputEl, result.message);
}
이런 장점이 생기겠죠?
- 각 함수가 { isValid, message } 형태로 일관된 형태로 결과를 반환해 코드를 좀 더 예측이 쉽고 안정적인 흐름을 가질수있도록 만들수있습니다.
- 객체를 통한 명확한 매핑을 사용해 조건문을 사용하는것보다 훨씬 코드의 의도를 명확히 드러낼 수 있습니다.
- 각 함수가 독립적이어서 개별적인 동작을 테스트하기쉽고, 수정 및 확장에 용이합니다.
- 재사용 가능한 로직을 쉽게 분리할 수 있습니다.
요구사항
기본
로그인
회원가입
심화
배포
https://statuesque-starship-bac495.netlify.app/
주요 변경사항
이번 미션에서는 관심사 분리에 의미를 두어 작업하였습니다.
scripts/#.js, scripts/#.js:
각 페이지(로그인, 회원가입)의 메인 로직 담당 — 페이지 단위 분리
scripts/utils/togglePassword.js:
비밀번호 보이기/숨기기 등 재사용 가능한 UI 로직 — UI 유틸 분리
scripts/validation/validation.js:
유효성 검사 전반을 담당하는 모듈 — 비즈니스 로직 분리
scripts/validation/validators.js:
개별 필드(이메일, 비밀번호 등)의 구체적인 유효성 검사 함수들 — 기능 단위 분리
scripts/validation/formUtils.js:
전체 폼 유효성 검사, 에러 메시지 처리 등 — 폼 처리에 특화된 도우미 분리
스크린샷