-
Notifications
You must be signed in to change notification settings - Fork 35
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
[서지훈]sprint4 #130
The head ref may contain hidden characters: "Basic-\uC11C\uC9C0\uD6C8-sprint4"
[서지훈]sprint4 #130
Conversation
@@ -36,23 +36,30 @@ | |||
<div class="input-item"> | |||
<label for="password">비밀번호</label> | |||
<div class="input-wrapper"> |
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.
input-wrapper
태그의 CSS 스타일을 모두 없애도 아무런 변화가 일어나지 않는데 그렇다면 이 div
를 없애도 되지 않을까요? 어떻게 생각하세요 ?
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.
flex-direction : column으로 배치 시키면 좋겠다 싶어서 넣었는데, 다시 확인해보니깐 없어도 동작이 그대로 되네요...ㅎ 특별하게 크게 동작하는거 같지 않아서 빼도 괜찮을거같습니다!!
@@ -0,0 +1,37 @@ | |||
document.addEventListener("DOMContentLoaded",function(){ |
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.
스크립트 태그가 제일 아래에 있어서 DOM이 모두 로드된 후에 자바스크립트가 실행되긴 하지만 그래도 이렇게 작성해주시면 센스있는 코드가 되는것 같습니다 !
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.
input
이벤트일때 에러메세지를 지워주면 좀 더 나은 UX가 될 것 같습니다 !
|
||
emailInput.addEventListener("focusout", function(){ | ||
loginValidationResults.email = checkEmailValidation(); | ||
toggleSubmitButton(); |
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.
toggleSubmitButton
이 중복되어 등장을 하네요. form
태그에 input
이벤트 핸들러를 초기화시켜놓으면 어떨까요? 그러면 자식 태그에서 발생하는 input
이벤트를 버블링 페이즈에서 잡아서 button
을 활성화 시킬지 검사 할 수 있을 것 같아요.
테스트해봤는데 자식 태그에서 input
이벤트 발생시 로그 확인 할 수 있어요.
signupForm.addEventListener("input", (event) => {
console.log("captured");
});
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.
최대한 함수 여러개 쓰는 중복을 배재 시킬려고 했는데, 초기화를 생각 못했네요ㅠㅠ 한번 수정해보도록 하겠습니다!
toggleSubmitButton(); | ||
}); | ||
nicknameInput.addEventListener("focusout", function(){ | ||
loginValidationResults.nickname = checkNicknameValidation(); |
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.
const signupValidationResults = {
email: false,
nickname: false,
password: false,
passwordCheck: false,
};
해당 객체를 sign up 페이지에서 사용하시려고 정의하신것 같은데 loginValidation
객체를 사용하셨네요.
혹시 어떤 이유에서일까요?
제 생각에는 이렇게 전역변수를 사용하는것은 좋지 않은 것 같습니다.
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.
기본 flag를 초기화 해서 저장 해놓은 다음, 에러 메시지가 발생하지 않을때, true 변경해 그 값을 signupValidationResults에 하나하나 저장해서, 최종적으로 모든 값이 true일때, 버튼이 활성화 되는 형태로 구성했습니다!
function checkPasswordValidation(checkPasswordInput){ | ||
|
||
const passwordInput = document.getElementById("password"); | ||
const isPassword = passwordInput.value.trim(); |
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.
유저의 입력값을 trim
하는것은 좋은 방법입니다 ! 센스있네요 !
function errorDisplay(input, message) { | ||
input.style.border = "1px solid #F74747"; | ||
let errorElement = document.getElementById(input.id + '-error'); | ||
if (!errorElement) { | ||
errorElement = createErrorMessageElement(input.id + '-error', message); | ||
input.insertAdjacentElement('afterend', errorElement); | ||
} else { | ||
errorElement.textContent = message; | ||
} | ||
errorElement.style.display = "block"; | ||
} | ||
|
||
// 상태 초기화 함수 | ||
function errorReset(input) { | ||
input.style.border = "none"; | ||
const errorElement = document.getElementById(input.id + '-error'); | ||
if (errorElement) { | ||
errorElement.style.display = "none"; | ||
} | ||
} |
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.
공통된 로직을 잘 뽑아서 중복 코드를 없애주셨네요 ~ ! 잘 하셨습니다 !
function toggleSubmitButton() { | ||
const submitButton = document.getElementById("submit-button"); | ||
const loginAllValid = Object.values(loginValidationResults).every(value => value === true); | ||
const signUpAllValid = Object.values(signupValidationResults).every(value => value === true); | ||
|
||
if (loginAllValid) { | ||
submitButton.removeAttribute("disabled"); | ||
} | ||
else if(signUpAllValid){ | ||
submitButton.removeAttribute("disabled"); | ||
} | ||
else { | ||
submitButton.setAttribute("disabled", "disabled"); | ||
} | ||
} |
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.
toggleSubmitButton
이 로그인 버튼과 회원가입 버튼에 각각 사용되네요. 그것을 if
문으로 구분짓고 있습니다. 제 생각에는 로그인 페이지를 위한 버튼, 회원가입 페이지를 위한 버튼 각각 함수를 따로 만드는게 낫다고 생각합니다.
함수는 하나의 책임만을 가지는게 낫습니다.
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.
그럼 따로 분리해서 동작하도록 설계해보겠습니다!
이런것은 사용자 편의성과는 그다지 관계가 없어보입니다 🥲 그리고 제가 테스트를 해봤을때는 지훈님이 의도하신대로는 잘 동작하는데 어쩔때 이슈가 생기는지 잘 모르겠네요 . |
어느 부분을 부분의 중복코드를 줄이기로 하셨었나요? 원래의 코드가 궁금합니다 ~ ㅎㅎ |
스프린트 미션 수고하셨습니다 ! 잘 이해가 안가거나 추가적인 질문이 있다면 멘토링때 해주시거나 아니면 디스코드 상으로 질문을 해주세요 ~ |
위에 있는 토글버튼 관련 이벤트랑 유사하게 로그인에서 사용되는 비밀번호 유효성 검사 함수를 비밀번호 유효성 검사 함수 + 비밀번호 확인 검사 함수 이렇게 합치고 싶었다가 분리 했습니다! |
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
멘토에게