-
Notifications
You must be signed in to change notification settings - Fork 39
[박다인] sprint4 #134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The head ref may contain hidden characters: "Basic-\uBC15\uB2E4\uC778-sprint4"
[박다인] sprint4 #134
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| document.addEventListener("DOMContentLoaded", () => { | ||
| const emailInput = document.getElementById("email"); | ||
| const usernameInput = document.getElementById("username"); | ||
| const passwordInput = document.getElementById("password"); | ||
| const repeatInput = document.getElementById("password-repeat"); | ||
|
|
||
| const emailError = document.getElementById("email-error"); | ||
| const usernameError = document.getElementById("username-error"); | ||
| const passwordError = document.getElementById("password-error"); | ||
| const repeatError = document.getElementById('repeat-error'); | ||
| const form = document.querySelector("form"); | ||
| const loginButton = form.querySelector("button[type='submit']"); | ||
|
|
||
| const isSignup = usernameInput && repeatInput; | ||
|
|
||
| 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; | ||
|
Comment on lines
+16
to
+42
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이런 로직도 위에서 드린 코멘트와 마찬가지로 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;
});
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코드의 흐름이 훨씬 예측가능하고, 의도가 명확해지고, 간결해지지않았나요? |
||
|
|
||
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
|
|
||
| const state = { | ||
| emailValid : false, | ||
| usernameValid: !!usernameInput, | ||
| passwordValid: false, | ||
| repeatValid: !!repeatInput | ||
| } | ||
|
|
||
| function validateEmail() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 함수 내부 body를 보면 유효성 검사의 대상만 바뀔뿐 똑같은 내용이 반복되고있습니다.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #133 (comment) |
||
| const emailValue = emailInput.value.trim(); | ||
| emailInput.classList.remove("invalid"); | ||
|
|
||
| if(!emailValue) { | ||
| emailError.innerText = "이메일을 입력해주세요."; | ||
| emailInput.classList.add("invalid"); | ||
| state.emailValid = false; | ||
| } else if (!emailRegex.test(emailValue)) { | ||
| emailError.innerText = "잘못된 이메일 형식입니다."; | ||
| emailInput.classList.add("invalid"); | ||
| state.emailValid = false; | ||
| } else { | ||
| emailError.innerText = ""; | ||
| state.emailValid = true; | ||
| } | ||
|
|
||
| updateButtonState(); | ||
| } | ||
|
|
||
| function validateUsername() { | ||
| const usernameValue = usernameInput.value.trim(); | ||
| usernameInput.classList.remove("invalid"); | ||
|
|
||
| if(!usernameValue) { | ||
| usernameError.innerText = "닉네임을 입력해주세요."; | ||
| usernameInput.classList.add("invalid"); | ||
| state.usernameValid = false; | ||
| } else { | ||
| usernameError.innerText = ""; | ||
| state.usernameValid = true; | ||
| } | ||
|
|
||
| updateButtonState(); | ||
|
|
||
| } | ||
|
|
||
| function validatePassword() { | ||
| const passwordValue = passwordInput.value.trim(); | ||
| passwordInput.classList.remove("invalid"); | ||
|
|
||
| if(!passwordValue) { | ||
| passwordError.innerText = "비밀번호를 입력해주세요."; | ||
| passwordInput.classList.add("invalid"); | ||
| state.passwordValid = false; | ||
| } else if (passwordValue.length < 8) { | ||
| passwordError.innerText = "비밀번호를 8자 이상 입력해주세요."; | ||
| passwordInput.classList.add("invalid"); | ||
| state.passwordValid = false; | ||
| } else { | ||
| passwordError.innerText = ""; | ||
| state.passwordValid = true; | ||
| } | ||
|
|
||
| updateButtonState(); | ||
| } | ||
|
|
||
| function validatePasswordRepeat() { | ||
| const repeatValue = repeatInput.value.trim(); | ||
| const passwordValue = passwordInput.value.trim(); | ||
| repeatInput.classList.remove("invalid"); | ||
|
|
||
| if(repeatValue !== passwordValue) { | ||
| repeatError.innerText = "비밀번호가 일치하지 않습니다."; | ||
| repeatInput.classList.add("invalid"); | ||
| state.repeatValid = false; | ||
| } else { | ||
| repeatError.innerText = ""; | ||
| state.repeatValid = true; | ||
| } | ||
|
|
||
| updateButtonState(); | ||
| } | ||
|
|
||
|
|
||
| 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"; | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
|
Comment on lines
+128
to
+157
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 함수에서 하는 일이 너무 많고, 네이밍도 모호해서 유지보수하기가 힘들것같아요.
|
||
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.
변수명을 isSignup => isSignupPage로 고쳐주시는게 좋을것같고,
usernameInput, repeatInput 두개 다 DOM에서 존재하는지 여부를 가지고 회원가입페이지인지 아닌지 판단하는 로직은 안정적이지 않을것같아요. 안정적이지않다고 생각한 이유는
로직이 DOM 구조에 의존적이예요.
디자인이 변경되거나 다른 방식으로라도 마크업이 변경되면 로직이 깨질 수 있어요.
확장성 문제:
새로운 필드가 추가되거나 제거될 때마다 로직을 수정해야해요.
따라서 해당 필드가 DOM내부에 있는지 여부보다는,
좀더 직관적이면서도 확실한 방식으로 현재 페이지가 회원가입 페이지인지 구분해줘야해요.
이런 방식은 어떨까요?
페이지 타입을 확인하는 함수를 만들고, 함수의 리턴값을 pageType을 반환받는것이 좀더 확실하고 안정적이겠죠?