-
Notifications
You must be signed in to change notification settings - Fork 24
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
[김현묵] sprint9 #127
base: next-김현묵
Are you sure you want to change the base?
[김현묵] sprint9 #127
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.
로그인 관련해서 컴포넌트를 잘 작성하시고 계신것 같아요.
여기서 AccessToken을 사용하고 계신데 SSO인증방식으로 구현한다면 토큰값을 지금처럼 0,1로 관리하는게 아니라, 제대로된 토큰을 사용하시게되면서 이값을 header로 처리하는 방식으로 구현하실것 같네요.
컴포넌트를 잘 분리해주신것도 좋은것 같습니다. 다만 좀더 가독성을 높히고 싶다면 props를 옵셔널로 관리하는 것 보다는 명확히 분리해주시는게 더 좋을것 같아요. 관련된 내용도 멘토링에서 한번 언급되면 좋을것 같아요
email: '', | ||
password: '', | ||
}); | ||
const condition = [!isEmail(form.email), form.password.length < 8]; |
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.
로그인을 하기위한 조건을 배열에 담아두신것 같아요.
배열에 담으면 어떤 인덱스가 어떤 값의 validation인지 판별하기 어려울것 같은데, 배열로 관리하시는 이유가 있을까요?
await login(form); | ||
router.push('/'); | ||
} else { | ||
alert('값을 제대로 입력해주세요'); |
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.
마지막 return은 의미가 없는 코드일 수 있을것 같아요. 아래 진행되는 코드가 없어서요!
) => void; | ||
maxLength?: number; | ||
children?: any; | ||
} | ||
|
||
export function LayoutInput({ |
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을 구성하실때 그에대한 에러처리, 에러메세지, 이벤트, children까지 모두 한번에 처리하기위해서 props를 구성해주신 것 같아요. 하지만 거의모든 props가 optional이기 때문에 컴포넌트 내부에서는 항상 props가 있는지 확인하는 로직이 발생할 것 같아요.
구현해주신 내용을 자세히 살펴보면 input, textarea, errormessage, label등 여러개의 컴포넌트로 분리할 수 있는 여지가 있을것 같아요. 이러한것을 분리하기 위한 패턴트로 Compound 패턴이라는것이 존재합니다.
어떻게하면 컴포넌트가 원하는 props만 받아서 사용할 수 있을지 고민해보시면 좋을 것 같아요
이건 멘토링시간에 공유할 수 있을면 좋을 것 같네요!
@@ -1,44 +1,47 @@ | |||
import { useEffect, useState } from "react"; |
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.
커스텀 훅들을 파일로 분리해주신것 좋습니다. 다만 이러한 훅들이 사용처에 맞게 파일이 분리가 되어있으면 좋을 것 같아요
{children} | ||
<Footer /> | ||
<Provider> | ||
<Header></Header> |
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.
(사소) children이 없을땐 <Header />
이렇게 작성해주셔도 됩니다!
export function AuthProvider({ children }: PropsWithChildren) { | ||
const [token, setToken] = useState(0); | ||
// const router = useRouter(); | ||
useEffect(() => {}, [token]); |
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.
토큰이 변경될때 useEffect가 동작하는데, 내부에 아무것도 없는것같아요. 의미없는 코드가 될것 같습니다!
queryFn: getUser, | ||
staleTime: 60 * 60 * 1000, | ||
enabled: !!token, | ||
}); |
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.
react-query를 통해서 get을 활용해주셨근요! queryKey가 변경되기 전까지 캐싱하기때문에, token을 queryKey에 넣어주신것 좋습니다!
password: string; | ||
}; | ||
export default function Login() { | ||
const { login } = useAuth(); |
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.
로그인/로그아웃 로직을 여러곳에서 사용하기때문에 컨텍스트로 사용해주신것도 아주 좋습니다.
이러한 로직은 한번 생성되면 다시 생성될 일이 없기에 전역 컨텍스트로 감싸주어도 성능상으로 문제가 발생할 확률이 적을것 같아요.
자주 사용하는 방식이고 잘 하신것 같습니다.
next 폴더가 현재 진행중인 fe 파일 들입니다.