-
Notifications
You must be signed in to change notification settings - Fork 4
[feat] 회원가입 페이지 구현 (/signup) #57
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
Conversation
✅ Deploy Preview for thejulge1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
almighty55555
left a comment
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.
수고하셨습니다!
src/pages/SignupPage.tsx
Outdated
| setFormData({ | ||
| email: "", | ||
| password: "", | ||
| confirmPassword: "", | ||
| userType: "employee", | ||
| }); |
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.
finally에서 setFormData()를 호출해 초기화를 하고 있는데 이 경우라면 서버 요청이 실패해도 폼이 초기화되어 사용자 입장에서는 에러가 났는데 입력값이 사라져서 재입력을 해야 하는 불편한 UX가 될 것 같습니다.
성공했을 경우에만 초기화를 시켜주는 건 어떨까요?
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.
넵 성공했을 때만 초기화를 시켜주는 것이 좋을 것 같습니다👍
src/pages/SignupPage.tsx
Outdated
| if (field === "email") { | ||
| setErrors((prev) => ({ | ||
| ...prev, | ||
| email: e.target.value.includes("@") |
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.
includes("@")만으로는 잘못된 형태가 많이 통과될 것 같습니다. (도메인이 없는 경우, @가 두 번 사용된 경우 등) 간단한 정규식을 사용해서 이메일 포맷을 좀 더 안정적으로 검증하는 게 어떨까요?
일례로 아래와 같이 작성할 수 있다고 합니당
const isValidEmail = /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(e.target.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.
해당 부분도 더 안전하게 검증하는 것이 좋을 것 같다고 생각합니다👍
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.
유효성 검사랑 상태 업데이트 로직이 너무 뭉쳐 있어서 따로 훅으로 추출하는 것도 좋을 것 같습니다. 이것은 그냥 저의 의견 ㅎㅎ입니당
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.
리뷰 주신 부분들 반영하여 코드 수정하도록 하겠습니다😊
jeonghwanJay
left a comment
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.
수고하셨습니다 !
src/pages/SignupPage.tsx
Outdated
| if (field === "email") { | ||
| setErrors((prev) => ({ | ||
| ...prev, | ||
| email: e.target.value.includes("@") |
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.
@만 확인하는 것은 이메일 형식의 최소 조건을 충족하지만, 실제 이메일 주소를 충족하지 않을 수 있을 것 같습니다.!
예를 들어, "abx@"나 "@naver.com" 같은 경우는 유효하지 않지만 @가 포함되어 있어 에러를 발생시키지 않을 것 같습니다.
정규식을 사용해서 이메일 형식을 검증해도 좋을 것 같습니다 ~!
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.
넵 해당 부분 수정하도록 하겠습니다!👍
#️⃣연관된 이슈
📝 PR 유형
📝작업 내용
SignupPage컴포넌트 구현ShopPage로 이동ProfilePage로 이동스크린샷 (선택)
💬리뷰 요구사항(선택)