-
Notifications
You must be signed in to change notification settings - Fork 39
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
[서미영] Sprint11 #278
The head ref may contain hidden characters: "Next-\uC11C\uBBF8\uC601-sprint11"
[서미영] Sprint11 #278
Conversation
type ValidationFunction = (input: string) => boolean; | ||
|
||
export interface AuthFormProps { | ||
mode: "login" | "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.
문자열 대신 더 특정적인 타입으로 해주신 부분 좋습니다! 👍👍
} | ||
|
||
export interface PageConfigType { | ||
mode: "login" | "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.
mode에 대한 타입값인 "login" | "signup"
가 다시 쓰이고 있으므로 별도의 type
으로 분리해서 공통화하면 좋을 것 같아요.
const router = useRouter(); | ||
const queryMode = router.query.mode; | ||
|
||
const mode: AuthFormProps["mode"] = |
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.
types/AuthTypes.ts 파일에서 드린 피드백과 비슷한 내용인데요. mode의 경우 여러군데에서 쓰이고 있으므로 별도 타입으로 분리하는 것이 좋아보여요
// console.log("사용자:", user); | ||
// console.log("액세스 토큰:", accessToken); | ||
// console.log("리프레시 토큰:", refreshToken); |
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.
콘솔은 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.
다만, 현실적으로 통신을 통해 가져온 값들을 체크하면서 개발하면 편리한 부분이 있는 것은 사실입니다. 🤔 만약, 개발 환경에서 제한적으로 콘솔을 보고 싶은 니즈가 있다면 아래와 같이 작성할도 있습니다.
// console.log("사용자:", user); | |
// console.log("액세스 토큰:", accessToken); | |
// console.log("리프레시 토큰:", refreshToken); | |
if (process.env.NODE_ENV === 'development') { | |
console.log("사용자:", user); | |
console.log("액세스 토큰:", accessToken); | |
console.log("리프레시 토큰:", refreshToken); | |
} |
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.
또한, 별도의 dev logger 의 역할을 하는 함수를 만든다면 조금 더 편하게 작성할 수 있고, 개발 로그 전체를 컨트롤 할 수 있어 조금 더 안전할 것 같네요. (다만, 실제 프로덕션 환경에서 자유롭게 쓰는 것은 고민해봐야 할 것 같습니다.)
const devLog = (...args: any[]): void => {
if (process.env.NODE_ENV === 'development') {
console.log(...args);
}
};
import axios from "axios"; | ||
|
||
const instance = axios.create({ | ||
baseURL: "https://panda-market-api.vercel.app/", |
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.
현재는 baseURL이 각각의 api파일에서 관리되고 있어 조금 아쉽습니다. 실제 개발을 진행하게 되면 baseURL이 바뀌는 경우가 꽤 흔하기 때문입니다. 백엔드도 개발을 진행하면서 url 자체가 바뀔수도 있고, 아니면 환경에 따라 url이 변경되게 됩니다. (개발 환경, 테스트 환경, QA 환경 등)
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.
이런식으로 상수로 관리하는 것이 더 좋은 방식이라고 볼 수 있을 것 같아요.
// constants/constants.ts
const baseURL = 'https://panda-market-api.vercel.app/'
@@ -21,7 +22,7 @@ export default function SearchInput({ | |||
placeholder="검색할 상품을 입력해주세요" | |||
onKeyDown={handleKeyDown} | |||
/> | |||
<img className={styles.search} src={searchImg.src} alt="검색 돋보기" /> | |||
<Image className={styles.search} src={searchImg} alt="검색 돋보기" /> |
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의 Image 컴포넌트 사용해 주신 부분 좋습니다. 👍
{!isLogin && <Button href="auth/login">로그인</Button>} | ||
{isLogin && <Image src={userImage} alt="유저 로그인 프로필" />} |
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.
각각, isLogin이 truthy, falsy할 때 보여주는 컴포넌트이다보니, 이 부분은 삼항연산자가 더 어올리는 것 같아요.
([ | ||
key, | ||
{ | ||
href, | ||
target, | ||
rel, | ||
img: { src, alt, width }, | ||
}, | ||
]) => ( |
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.
다만, Links가 객체라서 map으로 사용할 때 depth가 깊어져 사용하는 부분에서는 약간의 불편함도 있는 것 같습니다. 아예 Links를 배열로 선언하면 조금 더 편하게 쓸 수 있을 것 같아요.
PASSWORDCONFIRMATION = "passwordConfirmation", | ||
} | ||
|
||
export const fields: { [id: string]: FieldInfo } = { |
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.
Record를 사용해주시면 더 좋을 것 같습니다.
export const fields: { [id: string]: FieldInfo } = { | |
export const fields: Record<FIELDTYPE, FieldInfo> = { |
import { useRouter } from "next/router"; | ||
import styles from "./Auth.module.scss"; | ||
import Button from "../Button"; | ||
import { useForm, SubmitHandler } from "react-hook-form"; |
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.
RHF
(react-hook-form)을 잘 사용해 주셨습니다 👍
미영님, 이번주 과제 제출도 수고 많으셨습니다. 적절한 공통화와 react-hook-form 사용이 돋보이는 내용이었네요. 회원가입 페이지의 경우 로직이 많아 복잡해 지기 쉬운데 적절한 모듈화를 통해 가독성이 좋은 부분이 아주 좋았습니다. 👍👍👍 |
다만, 이번 과제 요구사항은 React 구현인데, 연습을 위해 별도로 Next.js로 구현하신건지 궁금하네요. 🤔 |
요 부분은 리뷰 완료하고 나서 확인했네요. 😲 |
setVisible(updatedVisible); | ||
onPasswordVisible(updatedVisible); | ||
}; | ||
|
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.
주소의 변경에 따라 isVisible 기본 값을 false로 설정해주는 로직이 필요합니다.
signup과 login 주소를 오가는 과정에서 password 에 해당하는 TogglePassword 컴포넌트는 최초랜더링 이후 리랜더링만 진행되고 있습니다. 이 과정에서 이전에 선택한 상태값이 보존되고 있으므로 페이지가 변화하였을 때는 기본 상태를 변경하는 로직이 필요합니다. 😁
const router = useRouter(); | |
useEffect(() => { | |
setVisible(false); | |
}, [router.asPath]); |
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.
요구사항
기본
심화
주요 변경사항
배포사이트
https://7-sprint-mission.vercel.app/
멘토에게
초기화를 하고 싶은데 useEffect로 주소 변경시 isVisible를 false로 업뎃하게 하거나 클린 함수 써도 초기화가 안되는데 방법이 있을까요?
밑에는 참고 스샷입니다~