-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feat: #5 회원가입 페이지 생성 및 라우터 설정 #6
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.
1차 리뷰 완료했습니다. 수고하셨습니다
아직 디자인이 확정되지 않아서 뼈대 내용이 변경되여야할 부분이 생길 것 같아요. 이후 변경점이 생기면 적용 부탁드립니다!
validation 부분이나 에러 메세지 부분은 이후 로직 작성하실 때 추가되는거죠?
const router = createBrowserRouter([ | ||
// { | ||
// path: '/', | ||
// element: , | ||
// }, | ||
{ | ||
path: '/signup', | ||
element: <SignUp />, | ||
}, | ||
]); | ||
|
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.
라우터 객체 기본 골자는 잘 만드신 것 같아요! 지금은 회원가입 라우터 설정만 들어가 있어서 이걸로 충분한 것 같습니다. 👍
<h1>비밀번호</h1> | ||
<input | ||
type="password" | ||
name="password" | ||
id="password" | ||
required | ||
className="block w-full rounded-md border-0 py-1.5 pl-7 pr-20 text-gray-900 ring-1 ring-inset ring-gray-300 placeholder:text-gray-400 focus:ring-2 focus:ring-inset focus:ring-indigo-600 sm:text-sm sm:leading-6" | ||
/> |
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.
h1보다 label로 관련된 input과 묶어주는게 좋을 것 같아요. label을 꼭 사용해야는 것은 아니지만, 사용 시 웹 접근성과 사용자 경험을 향상시켜주는 이점이 있습니다.
- 웹 접근성: 스크린 리더 지원
- 사용자 경험: label 클릭시 컨트롤 요소가 포커스되거나 접근 가능하게 해줌
🔗참고 내용
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.
안 그래도 회원가입 다른 페이지들 참고해보니 다들 label 사용하시길래 그렇구나 했는데 이유가 있었던 거군요! 앞으로 회원가입 로그인 페이지 작성할 때 label 태그 사용해보도록 하겠습니다!
<input | ||
type="text" | ||
name="website" | ||
id="website" | ||
className="block w-full rounded-md border-0 py-1.5 pl-7 pr-20 text-gray-900 ring-1 ring-inset ring-gray-300 placeholder:text-gray-400 focus:ring-2 focus:ring-inset focus:ring-indigo-600 sm:text-sm sm:leading-6" | ||
/> | ||
<h1>소속</h1> | ||
<input | ||
type="text" | ||
name="company" | ||
id="company" | ||
className="block w-full rounded-md border-0 py-1.5 pl-7 pr-20 text-gray-900 ring-1 ring-inset ring-gray-300 placeholder:text-gray-400 focus:ring-2 focus:ring-inset focus:ring-indigo-600 sm:text-sm sm:leading-6" | ||
/> |
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에 관련하여 대부분 유사한 로직을 반복하는 경향이 있다고 생각합니다. 예를 들어 onChange를 통해 변화하는 값을 지속적으로 추적하는 등의 내용이 그렇다고 생각합니다. 그래서 이를 공통 로직으로 추출하여 커스텀 훅(useInput)을 만들어서 관리해보는건 어떨까요?
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.
고려 중인 내용인데 바로 짚어주셨네요👍 커스텀 훅 만들어서 중복 제거해보도록 하겠습니다
type="text" | ||
name="website" | ||
id="website" | ||
className="block w-full rounded-md border-0 py-1.5 pl-7 pr-20 text-gray-900 ring-1 ring-inset ring-gray-300 placeholder:text-gray-400 focus:ring-2 focus:ring-inset focus:ring-indigo-600 sm:text-sm sm:leading-6" |
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.
pl-7과 pr-20을 설정한 이유는 무엇인가요? 크기를 지정하기 위한 내용이라면 padding left, right보다 width, height 등으로 조절하는게 좋지 않을까요?
PR Type
What kind of change does this PR introduce?
Related Issues
#5 회원가입 기능 개발
What does this PR do?
/signup
경로와 회원가입 페이지를 연결했습니다.Other information
라우터 사용법을 공부하며 참고한 블로그입니다.
페이지나 기능이 추가되면 중첩 라우팅이나 다양한 데이터 API를 라우터 코드에 적용해 보도록 하겠습니다.