-
Notifications
You must be signed in to change notification settings - Fork 44
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
[임상훈] week6 #251
base: part1-임상훈
Are you sure you want to change the base?
The head ref may contain hidden characters: "part1-\uC784\uC0C1\uD6C8-week6"
[임상훈] week6 #251
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.
고생 많으셨습니다!
사실 이전 받아왔던 코드리뷰에서 어떤 내용들을 중점으로 설명드렸는지는 모르겠으나,
제가 생각할 때 중요하다 판단하는 관심사의 분리 차원에서 피드백을 드렸는데요.
전반적인 설명은 오늘 멘토링에서 진행한 내용 참고해주시면 좋을 것 같구,
코드레벨 피드백을 통해 조금 더 디테일하게 제 생각 공유드렸으니 참고 부탁드립니다.
고생하셨습니다!
@@ -45,19 +55,47 @@ passwordToggleButton.addEventListener("click", () => | |||
|
|||
const signForm = document.querySelector("#form"); | |||
signForm.addEventListener("submit", submitForm); | |||
|
|||
function submitForm(event) { |
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.
submitForm
은 사실 submit 이벤트가 발생되면 어떻게 처리할지를 정의해주는 함수이죠? 따라서 submitForm
이라는 네임보다는 onFormSubmit
또는 handleFormSubmit
이라는 함수 이름이 더 좋아보입니다
let inputEmailPw = { | ||
email: emailInput.value, | ||
password: passwordInput.value, | ||
}; |
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.
변경되지 않을 변수인데 굳이 let
을 사용하는건 좋지 않아보여요.
let이 포함하고 있는 의미는 이후에 재할당이 된다는 뜻을 지니고 있어서요
const로 선언해 두는건 어떠신가요?
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.
또한 inputEmailPw
라는 이름보단, 로그인 관련 정보입을 표현하는 변수명이 더 좋아보입니다
fetch("https://bootcamp-api.codeit.kr/api/sign-in", { | ||
method: "POST", | ||
headers: { | ||
"Content-Type": "application/json", // 이 부분 추가 | ||
}, | ||
body: JSON.stringify(inputEmailPw), | ||
}) | ||
.then((response) => { | ||
if (response.ok) { | ||
window.location.href = "/folder"; | ||
} else { | ||
throw new Error("Server responded with an error!"); | ||
} | ||
}) | ||
.catch((error) => { | ||
console.error(error); | ||
setInputError( | ||
{ input: emailInput, errorMessage: emailErrorMessage }, | ||
"오류가 발생했습니다. 다시 시도해주세요." | ||
); | ||
}); | ||
} |
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.
실제 로그인 액션을 처리하는 녀석을 분리시켜보면 어떨까요? 오늘 우리 멘토링떄 이야기한 내용 참고해보면 좋을 것 같아요!
fetch("https://bootcamp-api.codeit.kr/api/check-email", { | ||
method: "POST", | ||
headers: { | ||
"Content-Type": "application/json", | ||
}, | ||
body: JSON.stringify({ email: email }), // 수정됨 | ||
}) | ||
.then((response) => response.json()) // 응답을 JSON으로 파싱 | ||
.then((data) => { | ||
if (data.isAvailable) { | ||
// 가정: 응답에 isAvailable 필드가 있다고 가정 | ||
removeInputError({ | ||
input: emailInput, | ||
errorMessage: emailErrorMessage, | ||
}); | ||
} else { | ||
setInputError( | ||
{ input: emailInput, errorMessage: emailErrorMessage }, | ||
"이미 사용 중인 이메일입니다." | ||
); | ||
} | ||
}) | ||
.catch((error) => { | ||
console.error("Error checking email:", error); | ||
setInputError( | ||
{ input: emailInput, errorMessage: emailErrorMessage }, | ||
"이메일 확인 중 오류가 발생했습니다." | ||
); | ||
}); |
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.
validateEmailInput
에서 처리하기에 너무 무게감이 큰 녀석이에요!
또한 단순 input 체크보단 데이터베이스에 존재하는지 여부를 체크하는 목적인만큼, 외부 함수로 구현해 활용하는게 좋겠어요.
음 만약 코드잇 요구사항에서 이메일 입력창 벗어날때마다 이 네트워크 통신을 통한 밸리데이션이 필요하다 명세했다면 어쩔 수 없지만, 보통 프로덕션 제품에서는 이런 기능은 회원가입 실행 후 체크하는 요소로 평가됩니다.
function validatePasswordInput(password) { | ||
if (password === "" || !isPasswordValid(password)) { | ||
setInputError( | ||
{ input: passwordInput, errorMessage: passwordErrorMessage }, | ||
"비밀번호는 영문, 숫자 조합 8자 이상 입력해 주세요." | ||
); | ||
return false; | ||
} | ||
removeInputError({ | ||
input: passwordInput, | ||
errorMessage: passwordErrorMessage, | ||
}); | ||
return true; | ||
} |
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.
validation과 ui controlling을 함꼐하고 있네요. 조금 더 분리시킬 수 있으면 좋겠어요!
isEmailInputValid && | ||
isPasswordInputValid && | ||
isConfirmPasswordInputValid |
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.
이렇게 뭉뜽그려 validation를 체크하게 되면, 어떤 요소로 인해 회원가입이 실패했는지 확인하기 어렵습니다.
따라서 validate~~~Input
이라는 녀석은 실제 값에 대해 validation만 처리하고, 그 건에 대해 어떻게 ui 업데이트를 해줄건지를 각각의 케이스에 따라 if 문으로 구현하는게 좋아보여요!
fetch("https://bootcamp-api.codeit.kr/api/sign-up", { | ||
method: "POST", | ||
headers: { | ||
"Content-Type": "application/json", | ||
}, | ||
body: JSON.stringify({ | ||
email: emailInput.value, | ||
password: passwordInput.value, | ||
}), | ||
}) | ||
.then((response) => { | ||
if (response.ok) { | ||
window.location.href = "/folder"; | ||
} else { | ||
throw new Error("Server responded with an error!"); | ||
} | ||
}) | ||
.catch((error) => { | ||
console.error(error); | ||
setInputError( | ||
{ input: emailInput, errorMessage: emailErrorMessage }, | ||
"오류가 발생했습니다. 다시 시도해주세요." | ||
); | ||
}); | ||
} | ||
location.href = "/folder"; |
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.
이렇게 구현하면 fetch, then then catch등이 실행되고 난 후에 location.href가 실행되는 순서가 보장되나요?
한번 비동기 코드 실행순서 체크 필요할 것 같습니다.
요구사항
기본 요구사항은 모두 충족하였습니다.
netlify에서 배포한 웹사이트 링크입니다!
https://65ffe21ceb2037ad7c1fae2f--moonlit-concha-24432a.netlify.app/
멘토에게