-
Notifications
You must be signed in to change notification settings - Fork 0
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(community): 로그인/회원가입 페이지 추가 #27
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
console.log(data); | ||
}; | ||
|
||
const [studentId, password, confirmPassword, name, email, phone] = useWatch({ |
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.
useWatch 대신 formState의 isValid를 이용하는 건 어떨까요?
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.
Copilot reviewed 5 out of 20 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- apps/community/package.json: Language not supported
- apps/community/src/app/(auth)/signin/page.css.ts: Evaluated as low risk
- pnpm-lock.yaml: Evaluated as low risk
- apps/community/src/schemas/sign-up-form-schema.ts: Evaluated as low risk
- apps/community/src/components/auth-button.tsx: Evaluated as low risk
- apps/community/src/components/auth-input.tsx: Evaluated as low risk
- apps/community/src/components/auth-footer.tsx: Evaluated as low risk
- apps/community/src/app/(auth)/signin/page.tsx: Evaluated as low risk
- apps/community/src/components/auth-header.tsx: Evaluated as low risk
- apps/community/src/components/sign-up-form.tsx: Evaluated as low risk
- apps/community/src/components/auth-button.css.ts: Evaluated as low risk
- apps/community/src/app/(auth)/signup/page.tsx: Evaluated as low risk
- apps/community/src/components/sign-in-form.tsx: Evaluated as low risk
- apps/community/src/components/auth-input.css.ts: Evaluated as low risk
- apps/community/src/components/auth-footer.css.ts: Evaluated as low risk
AuthInput은 직접 DS에 컴포넌트로 추가하여 해당 PR에 포함해도 좋을 거 같습니다. 어때요? |
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.
빠르게 작업하셨네요 👍👍 몇 가지 제안합니다.
- app/(auth)/components
해당 폴더에서 auth에 관련된 '공용' 컴포넌트가 선언되어 있는데요. 여기서 sign-in-form이나 sign-upform 등 signin, signup에서 사용되는 전용 컴포넌트가 해당 폴더에 포함하신 이유가 궁금해요.
저는 각각 페이지 폴더에 components에 선언되어야 한다고 생각합니다.
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.
👍👍 참고하여 DS에 반영하도록 할게요
const signInFormSchema = z.object({ | ||
studentId: z.string().min(1, { message: '학번을 입력해주세요.' }), | ||
password: z | ||
.string() | ||
.min(1, { message: '비밀번호를 입력해주세요.' }) | ||
.regex( | ||
/^(?=.*[a-zA-Z])(?=.*[0-9])(?=.*[~!@#$%^&*])[a-zA-Z0-9~!@#$%^&*]{8,15}$/, | ||
{ message: '올바른 비밀번호 형식이 아닙니다.' }, | ||
), | ||
}); |
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.
저도 동의합니다 😃
@m2na7 브랜치 최신으로 업데이트 했습니다 pull 당겨주시고, 리뷰한거 피드백 해주세요 |
input컴포넌트는 DS에 반영했고, 전체 파일 폴더구조(src/component) 변경했습니다 ! |
|
||
interface Props extends React.InputHTMLAttributes<HTMLInputElement> { | ||
label?: string; | ||
value?: 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.
value?: string; |
{label} | ||
</label> | ||
)} | ||
<input value={value} className={styles.input()} {...props} /> |
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 value={value} className={styles.input()} {...props} /> | |
<input className={styles.input()} {...props} /> |
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은 nesting-layouts를 사용하여 children으로 내려주는 방법으로 사용해주세요
ref: https://nextjs.org/docs/app/building-your-application/routing/layouts-and-templates#nesting-layouts
label?: string; | ||
value?: string; | ||
message?: string; | ||
className?: 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.
className?: 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.
LGTM
Summary
로그인&회원가입(auth) 페이지(/signin , /singup) 구현
Tasks
To Reviewer
로그인/회원가입 폼 컴포넌트를 각각 구현했어요.
Form
컴포넌트,useFormHandler
커스텀 훅으로 관련 로직을 분리해서 구현하는 방법도 있고, 실제로 구현해보았는데 구조가 지나치게 복잡해진다고 생각했어요.인증 관련 폼은 로그인/회원가입에만 사용되고, 공통 로직을 분리하는 과정에서 오버엔지니어링이 된다고 판단했고, 각각의 컴포넌트 (
SignInForm
,SignUpForm
)로 구현했어요.관련해서 의견주시면 반영해보도록 하겠습니다 😃
Screenshot