-
Notifications
You must be signed in to change notification settings - Fork 46
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
[김도훈] Sprint 11 #331
The head ref may contain hidden characters: "Next-\uAE40\uB3C4\uD6C8-sprint11"
[김도훈] Sprint 11 #331
Conversation
아직 구현 전이신거죠? 구현 완료되면 태그 부탁드려요~ |
@baeggmin 너무 늦게 완성이 되어서 죄송합니다ㅜㅜ심화 제외하고 구현완료해서 리뷰 해주셔도 좋을거 같습니다! |
@baeggmin 구현완료 했습니다! |
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 { useRouter } from "next/navigation"; | ||
import { signup, login } from "../api/api"; | ||
|
||
export default function CommonForm({ type }: AuthFormProps) { |
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.
중복되는 부분을 공통 컴포넌트화 하시려 한 것 같은데요, 이 영역은 오히려 분리해서 작성하는게 유지보수 측면에서 더 낫지 않을까 싶네요..🤔
하나의 컴포넌트에 로그인과 회원가입 모두 다루려고 하다보니 state 가 많아져 가독성이 떨어지고, handleSubmit 라는 하나의 함수 안에서 분기처리를 하게되면 나중에 더 복잡한 로직이 추가되었을 때 점점 더 읽기 어려운 코드가 될 것 같아요.
차라리 공통된 로직 부분을 custom hook 으로 분리해보시는건 어떨까요? email, password, showPassword 등의 로직은 훅으로 빼서 재사용하게 만들면 좋을 것 같습니다.
const [showRePassword, setShowRePassword] = useState(false); | ||
const router = useRouter(); | ||
|
||
useEffect(() => {}, [email, nickname, password, repassword]); |
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.
이 줄은 왜 필요한건가요?? 실수로 넣으신거겠죵?
const [isOpen, setIsOpen] = useState(false); | ||
const router = useRouter(); | ||
|
||
useEffect(() => { |
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.
멘토링 시간에 언급드렸듯이 client component 에서 localstorage 에 접근할 때 간헐적으로 undefined 에러가 날 수 있습니다.
이를 방지하기 위해 localStorage 가 undeifned 인지 확인하는 조건 하나가 추가되면 좋을 것 같네요.
import Image from "next/image"; | ||
import { Mcard } from "../type/type"; | ||
|
||
export default function MainCard({ |
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.
공통 컴포넌트 잘 만들어주셨네요! 👍
@@ -29,3 +31,17 @@ export interface Comment { | |||
nickname: 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.
공용으로 사용하는 인터페이스&타입을 하나의 파일에서 관리하는건 좋은 방법이긴 하지만, 기능별 or 페이지별 colocation 을 지켜주는 것도 중요합니다. 지금은 프로젝트에서 사용되는 모든 공용 인터페이스가 한 파일에 있다보니, 타입을 확인할때마다 파일을 널뛰기 하며 움직여야해서 조금 불편해질 수 있을 것 같아요.
Mcard 의 경우에는 MainCard 컴포넌트에 선언해두는게 더 관리하기 쉬울 것 같네용.
요구사항
기본
심화
사진
멘토에게