-
Notifications
You must be signed in to change notification settings - Fork 35
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 #314
The head ref may contain hidden characters: "Next-\uC774\uC2B9\uD604-sprint11"
[이승현] Sprint11 #314
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.
수고많으셨습니다!!
개발하시면서 어? 이 코드 아까 썼던것같은데? 가 생각나면 가까운 상위 폴더로 추출해서 재사용 해보는 연습이 리팩터링 하는데에 도움이 되더라구요.
지금도 충분히 잘 하고계십니다. 힘내세요!!
const useDateFormat = () => { | ||
const format = useCallback((date: Date) => { | ||
const newDate = new Date(date); | ||
const formatDate = `${newDate.getFullYear()}.${String( | ||
newDate.getMonth() + 1 | ||
).padStart(2, "0")}.${String(newDate.getDate()).padStart(2, "0")}`; | ||
return formatDate; | ||
}, []); | ||
|
||
return { format }; | ||
}; |
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.
반복 사용되는 기능을 잘 분리해주셨습니다!
아마 useCallback을 쓰기위해서 커스텀 훅으로 만드신것같아요. dateFormat을 반환하는 함수라서 많은 계산이 필요한 함수는 아닐것같아요. 그래서 useCallback없이 utill 함수로 분리해도 될것같아요.
@@ -8,6 +8,7 @@ import Link from "next/link"; | |||
import { getArticles } from "@/lib/articleApi"; | |||
|
|||
const BestArticles = () => { | |||
const MAX_COUNT = 9999; |
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 const API_ENDPOINTS = { | ||
getArticles: ( | ||
page: number, | ||
pageSize: number, | ||
orderBy?: string, | ||
keyword?: string | ||
): string => { | ||
const orderByQuery = orderBy ? `&orderBy=${orderBy}` : ""; | ||
const keywordQuery = keyword ? `&keyword=${keyword}` : ""; | ||
return `/articles?page=${page}&pageSize=${pageSize}${orderByQuery}${keywordQuery}`; | ||
}, | ||
getArticle: (articleId: number): string => `/articles/${articleId}`, | ||
getComments: (articleId: number, limit: number): string => | ||
`/articles/${articleId}/comments?limit=${limit}`, | ||
addUser: "/auth/signUp", | ||
loginUser: "/auth/signIn", | ||
}; |
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.
address용으로 잘 분리 해 주셨는데,
querystring은 컴포넌트에서 추가,수정,삭제, 업데이트 하는건 어떨까요? address에서는 queryString에 대한 내용을 몰라도 되고, 컴포넌트에서 직접 추가,수정,삭제, 업데이트하면 주소 객체를 관리하기 좀더 수월할 것같아요.
만약 지금의 방식대로면 getArticles에 query가 늘어날 때마다 변수가 하나씩 늘어나거나,
수정될 때마다 문자열에 오타는 없는지 찾아가면서 유지보수하는 시간이 늘어날 것같아요.
next.js 공식문서에 searchParams, params사용법 입니다 참고 해 보세요 :-)
const instance = axios.create({ | ||
baseURL: "https://panda-market-api.vercel.app/", | ||
baseURL: process.env.NEXT_PUBLIC_API_BASE_URL, | ||
}); |
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.
인스턴스 잘 만들어주셨습니다!
instance함수가 호출되려면 이 파일이 import 되어있어야 하는데, main.ts 혹은 최상단에서 import 하고있는지, interceptor는 잘 동작하는지 한번 확인 해주시는것도 좋아요 :-)
} catch (error) { | ||
console.error("Error in signup: ", error); | ||
throw error; |
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.
ctach 문에서 다루고 있는건 없지만
회원가입이 실패하는 조건이 어떤게 있는지 한번 생각해보는것도 도움이 될것같아요 :-)
<button className={styles.commentSubmitBtn}>등록</button> | ||
<button | ||
className={styles.commentSubmitBtn} | ||
disabled={commentValue ? false : true} |
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.
문자열에 공백도 문자열의 길이로 취급됩니다.
commentValue가 있는지 없는지로 판단하기에는 조금 범위가 넓은것같아요
const onSubmit = async (data: LoginUserRequest) => { | ||
try { | ||
const res: { data: AuthResponse } = await loginUser(data); | ||
console.log(res); |
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.
commit 하기전에 console.log 한번씩 훝어주시는 습관을 가지시면 좀더 편하실꺼에요 :-)
}, | ||
pattern: { | ||
value: | ||
/^[0-9a-zA-Z]([-_\.]?[0-9a-zA-Z])*@[0-9a-zA-Z]([-_\.]?[0-9a-zA-Z])*\.[a-zA-Z]{2,3}$/i, |
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.
정규표현식도 변수에 담거나, 정규표현식에 입력값을 test해서 반환된 결과를 사용하는것도 가능합니다.
JSX를 쭉~ 읽다가 이렇게 복잡한 코드가 나오면 코드를 읽어내려가는데에 중간에 텈! 막히기도 해서 변수에 담아서 추상화 해주는것도 읽기 좋은 코드를 만드는 작~은 방법입니다.
그리고 이 정규표현식이 어떤 조건인지 주석으로 추가 설명 해주면 더더욱 좋을것같아요!
예를들어
// @ 왼쪽은 영문 대소문자, 숫자, 특수문자(-_/.)가능, @오른쪽은 ~ 이런식으로 코드를 볼 사람을 위해서 남겨두면 이해하기 쉬울것같아요
{...register("password", { | ||
required: { | ||
value: true, | ||
message: "비밀번호를 입력해주세요.", | ||
}, | ||
minLength: { | ||
value: 8, | ||
message: "비밀번호를 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.
const passwordRules = {
required: {
value: true,
message: "비밀번호를 입력해주세요.",
},
minLength: {
value: 8,
message: "비밀번호를 8자 이상 입력해주세요.",
},
}
...
...
{...register("password", passwordRules)}
요렇게 추상화 해보는건 어떨까요?
{...register("email", { | ||
required: { | ||
value: true, | ||
message: "이메일을 입력해주세요.", | ||
}, | ||
pattern: { | ||
value: | ||
/^[0-9a-zA-Z]([-_\.]?[0-9a-zA-Z])*@[0-9a-zA-Z]([-_\.]?[0-9a-zA-Z])*\.[a-zA-Z]{2,3}$/i, | ||
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.
login이랑 같은 코드를 쓰는 부분이 있는것같아요. 추출해서 반복사용해보는건 어떨까요??
요구사항
기본
회원가입
로그인
메인
멘토에게