[박다인] sprint4 #134
[박다인] sprint4 #134
Conversation
addiescode-sj
left a comment
addiescode-sj
left a comment
There was a problem hiding this comment.
수고하셨습니다!
DOM 종속적으로 코드를 짜시는 습관이 있으신것같아요.
코멘트 보시고 천천히 하나씩 리팩토링해봅시다 👍
주요 리뷰 포인트
- DOM 종속적인 로직 연결 제거
- 유지보수를 고려한 개발
| const form = document.querySelector("form"); | ||
| const loginButton = form.querySelector("button[type='submit']"); | ||
|
|
||
| const isSignup = usernameInput && repeatInput; |
There was a problem hiding this comment.
변수명을 isSignup => isSignupPage로 고쳐주시는게 좋을것같고,
usernameInput, repeatInput 두개 다 DOM에서 존재하는지 여부를 가지고 회원가입페이지인지 아닌지 판단하는 로직은 안정적이지 않을것같아요. 안정적이지않다고 생각한 이유는
-
로직이 DOM 구조에 의존적이예요.
디자인이 변경되거나 다른 방식으로라도 마크업이 변경되면 로직이 깨질 수 있어요. -
확장성 문제:
새로운 필드가 추가되거나 제거될 때마다 로직을 수정해야해요.
따라서 해당 필드가 DOM내부에 있는지 여부보다는,
좀더 직관적이면서도 확실한 방식으로 현재 페이지가 회원가입 페이지인지 구분해줘야해요.
이런 방식은 어떨까요?
페이지 타입을 확인하는 함수를 만들고, 함수의 리턴값을 pageType을 반환받는것이 좀더 확실하고 안정적이겠죠?
/* 페이지 타입 확인 */
const getPageType = () => {
// URL 경로나 현재 페이지의 특정 요소를 통해 판단
const path = window.location.pathname;
const isSignupPage = path.includes('signup') ||
document.querySelector('form[action*="signup"]') ||
document.querySelector('h1, h2').textContent.toLowerCase().includes('회원가입');
return isSignupPage ? 'signup' : 'login';
};
const pageType = getPageType();
const isSignupPage = pageType === 'signup';| const togglePasswordIcon = document.getElementById("togglePassword"); | ||
| const togglePasswordRepeatIcon = document.getElementById("togglePassword-repeat"); | ||
|
|
||
| if (togglePasswordIcon && passwordInput) { | ||
| togglePasswordIcon.addEventListener("click", () => { | ||
| const isHidden = passwordInput.type === "password"; | ||
|
|
||
| passwordInput.type = isHidden ? "text" : "password"; | ||
|
|
||
| togglePasswordIcon.src = isHidden | ||
| ? "images/btn_visibility.png" | ||
| : "images/btn_unvisibility.png"; | ||
| }); | ||
| } | ||
| if (togglePasswordRepeatIcon && repeatInput) { | ||
| togglePasswordRepeatIcon.addEventListener("click", () => { | ||
| const isHidden = repeatInput.type === "password"; | ||
|
|
||
| repeatInput.type = isHidden ? "text" : "password"; | ||
|
|
||
| togglePasswordRepeatIcon.src = isHidden | ||
| ? "images/btn_visibility.png" | ||
| : "images/btn_unvisibility.png"; | ||
| }); | ||
| } | ||
|
|
||
| loginButton.disabled = true; |
There was a problem hiding this comment.
이런 로직도 위에서 드린 코멘트와 마찬가지로 DOM 종속적이네요!
마크업이 변경되면, 로직이 깨집니다.
아래 예시와 같이 토글과 관련된 값을 상수로 관리하고 data 속성을 추가해서 바꿔주는건 어떨까요?
/* 비밀번호 토글 관련 상수 */
const PASSWORD_TOGGLE_CONFIG = {
icons: {
visible: 'images/btn_visibility.png',
hidden: 'images/btn_unvisibility.png'
},
selectors: {
password: '[data-password-toggle]',
icon: '[data-password-toggle-icon]'
}
};
/* 비밀번호 토글 이벤트 핸들러 */
const handlePasswordToggle = (event) => {
const toggleButton = event.currentTarget;
const input = toggleButton.closest('.input-group').querySelector('input[type="password"], input[type="text"]');
const isHidden = input.type === 'password';
input.type = isHidden ? 'text' : 'password';
toggleButton.src = isHidden ? PASSWORD_TOGGLE_CONFIG.icons.visible : PASSWORD_TOGGLE_CONFIG.icons.hidden;
};그리고 토글 상태를 초기화하는 코드도 필요해지겠죠?
/* 비밀번호 토글 이벤트 초기화 */
const initPasswordToggle = () => {
const toggleButtons = document.querySelectorAll(PASSWORD_TOGGLE_CONFIG.selectors.icon);
toggleButtons.forEach(button => {
button.addEventListener('click', handlePasswordToggle);
});
};
// 페이지 로드 시 초기화
document.addEventListener('DOMContentLoaded', () => {
initPasswordToggle();
loginButton.disabled = true;
});There was a problem hiding this comment.
코드의 흐름이 훨씬 예측가능하고, 의도가 명확해지고, 간결해지지않았나요?
코멘트 내용을 전부 반영하시기보다는 DOM 종속적으로 코드를 작성하는게 왜 안좋은지 그 이유와 리팩토링 목적정도만 참고해가셔도 좋습니다! 천천히 하나씩 다듬어봐요 :)
| repeatValid: !!repeatInput | ||
| } | ||
|
|
||
| function validateEmail() { |
There was a problem hiding this comment.
함수 내부 body를 보면 유효성 검사의 대상만 바뀔뿐 똑같은 내용이 반복되고있습니다.
함수에 검사 대상과 관련된 인자를 전달해주고 같은 내용을 수행할수있게 리팩토링한다면 코드 중복을 크게 줄일수있겠죠?
There was a problem hiding this comment.
#133 (comment)
다른 분 PR에 드린 코멘트 예시 참고해보시면 좋을듯합니다! :)
| function updateButtonState() { | ||
| if (isSignup) { | ||
| loginButton.disabled = !(state.emailValid && state.passwordValid && state.usernameValid && state.repeatValid); | ||
| } else { | ||
| loginButton.disabled = !(state.emailValid && state.passwordValid); | ||
| } | ||
| } | ||
|
|
||
| emailInput.addEventListener("focusout", validateEmail); | ||
| passwordInput.addEventListener("focusout", validatePassword); | ||
|
|
||
| if (usernameInput && repeatInput) { | ||
| usernameInput.addEventListener("focusout", validateUsername); | ||
| repeatInput.addEventListener("focusout", validatePasswordRepeat); | ||
| } | ||
|
|
||
|
|
||
| form.addEventListener('submit', (e) => { | ||
| e.preventDefault(); | ||
| updateButtonState(); | ||
|
|
||
| if (!loginButton.disabled) { | ||
| if(isSignup) { | ||
| window.location.href = "login.html"; | ||
| } else { | ||
| window.location.href = "/items"; | ||
| } | ||
| } | ||
| }); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
해당 함수에서 하는 일이 너무 많고, 네이밍도 모호해서 유지보수하기가 힘들것같아요.
함수가 단일 책임을 가질수있게끔 아래 예시를 기준으로 작업을 나눠보고 필요한 시점에 사용될수있게끔 구조를 바꿔보는건 어떨까요?
- 이벤트 리스너 설정 및 초기화
- 유효성 검사
- UI 상태 업데이트
질문에 대한 답변
모듈화를 시도하신다면 같은 코드를 반복 작성하실 필요가 없어집니다. 다음 미션에서 모듈화를 시도하라는 구현 과제가 있을텐데 한번 시도해보시죠! 그리고 UI에 의존적인 코드 (DOM 조회, 이벤트 리스너 연결, UI 업데이트 등)은 페이지별 자바스크립트 파일을 따로 두시고 그 파일안에서 작성하시는게 관리에 수월할거예요 :) |
2 participants
요구사항
기본
로그인
회원가입
심화
주요 변경사항
사이트
🔗 https://panda-market-dain.netlify.app
스크린샷
멘토에게