-
Notifications
You must be signed in to change notification settings - Fork 4
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
[FE] feat: 회원가입, 로그인 페이지 구현 #32
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.
철민님! 서비스의 첫 시작일 회원가입, 로그인 페이지 구현하느라 고생 많으셨어요! 깔끔하게 잘 해주신 것 같아요 👍🏻 사소한 디테일까지 챙기신 것도 짱인데요? 짱철민!
별다른 버그는 없어서 approve 해둘게요! 다른 코멘트들은 소소한 제 의견이기에 참고만 해주셔도 될 것 같아요 :) 이번주도 파이팅 🔥
const MAX_AGE = 150; | ||
const MAX_MONTH = 12; | ||
const MAX_DAY = 31; | ||
|
||
const currentYear = new Date().getFullYear(); | ||
const years = Array.from({ length: MAX_AGE }, (_, index) => String(currentYear - index)); | ||
const months = Array.from({ length: MAX_MONTH }, (_, index) => String(index + 1)); | ||
const days = Array.from({ length: MAX_DAY }, (_, index) => String(index + 1)); |
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.
소소한 부분인데 2월은 29~31일이 존재하지 않으니 28일까지, 2024년은 윤년이라 2월 29일이 존재해서 29일까지 보여주고 싶으시다면 daysjs
를 사용해봐도 좋을 것 같아요! 예시로 들자면const date = daysjs('2024-02-29')
같은 경우 date.isValid()를 해보면 true가 나와요! 현재 선택된 날짜를 daysjs로 연-월-일
을 통해 인스턴스를 생성하면 해당 월의 마지막 날짜를 구하는 것도 쉬워지지 않을까 하는 생각이 들었어요 :)
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.
안 그래도 지금 상태에서는 월 상관 없이 무조건 31일까지 보여주고 있어서, 유효성 검사를 추가할 때 말씀해주신 부분도 적용해보려 해요. day.js
를 찾아보니 되게 작고 가벼우면서 날짜를 포맷팅하기도 쉬워서 사용해도 괜찮을 것 같아요..! 참고하겠습니다 ㅎㅎ
<S.PageContainer> | ||
<S.ContentWrapper> | ||
<S.LoginFormContainer> | ||
<S.MainLoginContainer> |
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.
Layout -> Container -> Wrapper 와 같은 구조로 내려가는걸 컨벤션으로 이해했는데 여러 요소를 묶으면 구조 상관없이 Container로 묶으면 되는 걸까요?? 💭
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.
제가 생각했던 Container, Wrapper의 기준은 각각 여러 요소를 묶느냐, 하나의 요소를 감싸느냐 였어요! 그래서 페이지의 레이아웃에 따라 적절하게 두 가지를 섞어서 사용하시면 될 것 같아요. 반드시 Container 안에 Wrapper가 들어갈 필요는 없다고 생각해요.
하나의 Content 요소 안에 비슷한 요소들이 여러 개 묶여있을 수 있으니까요 😄
const fadeInDown = keyframes` | ||
0% { | ||
opacity: 0; | ||
transform: translate3d(0, -100%, 0); | ||
} | ||
50%{ | ||
opacity: 0; | ||
transform: translate3d(0, -100%, -50%); | ||
} | ||
to { | ||
opacity: 1; | ||
transform: translateZ(0); | ||
} | ||
`; |
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.
애니메이션까지 추가해주는 센스 👍🏻 안그래도 애니메이션 관련해서 고민이 있었는데요, 다양하고 편리한 효과를 위해 framer를 사용해보는건 어떠실지 궁금해요!
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.
Framer Motion을 말씀하시는 것이 맞을까요?? 디스코드에는 애니메이션 효과가 거의 대부분 들어가 있어서 편리하게 사용하는 것도 괜찮을 것 같아요!
|
||
const handleYearSelect = (year: string) => { | ||
setSelectedYear(year); | ||
monthRef.current?.focus(); |
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.
focus 넘어가는 디테일까즤 👏🏻
label: string; | ||
isRequired?: boolean; | ||
error?: string; | ||
type?: React.InputHTMLAttributes<HTMLInputElement>['type']; |
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이면 type을 'text' | 'password' | 'email'만 명시해두는 건 어떠신지 의견을 여쭤보고싶어요! 의도를 좀 더 살리고 타입 안정성도 올릴 수 있지 않을까하는 생각이에요 :)
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.
그 세 가지 이외에는 사용되지 않으니 좋은 것 같습니다 :) 반영할게요!
import * as S from './styles'; | ||
import useDropdownInput from './useDropdownInput'; | ||
|
||
export type DropdownInputItem = string; |
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.
이건 컨벤션과 관련한 질문인데요 prop과 관련한 interface 이외의 경우에는 타입을 별도로 분리하지 않아도 될까요?? 아직 좀 헷갈리나봐요 😅
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.
DropdownInputItem
타입 같은 경우에는 해당 컴포넌트에서 사용되고 있기 때문에 별도의 파일로 분리하는 것이 오히려 코드의 응집성을 떨어뜨린다고 생각했어요!
타입 관련해서는 무조건 분리해야 한다는 컨벤션을 정하지는 않아서, 혜인님이 적절하게 판단하셔도 괜찮을 것 같아요
Framer Motion 적용
animation.webm |
이슈번호
요약(개요)
AuthCheckbox
,AuthDateInput
,AuthInput
,DropdownInput
작업 내용
▲ 로그인 화면
▲ 회원가입 화면
집중해서 리뷰해야 하는 부분
레이아웃 초기 뼈대 코드의 경우 임의로 작성했습니다. 만약 충돌하는 부분이 있다면 서로 코드를 공유해서 아예 충돌을 이 PR에서 해결하는 것도 괜찮을 것 같아요.
기타 전달 사항 및 참고 자료(선택)
/login
과/register
에서 페이지 확인 가능합니다.