-
Notifications
You must be signed in to change notification settings - Fork 37
[최성락] Sprint10 #301
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
[최성락] Sprint10 #301
The head ref may contain hidden characters: "Next-\uCD5C\uC131\uB77D-sprint10"
Conversation
Lanace
left a comment
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.
전반적으로 코드가 깔끔하고 일정한 형태를 갖추고 있어서 보기도 좋고 예상 가능한 코드여서 리뷰하기도 좋았네여ㅎㅎ!
개발하느라 고생 많으셨어요~!!
| .link { | ||
| text-decoration: none; | ||
| } |
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.
자주 등장하게 될텐데 저는 개인적으로 이런 스타일은 global에 적용해두긴 해요ㅎㅎ;;
예외적으로 몇개 있긴 한데, 링크에 underline도 그렇고 appearance도 그래요...!
한번 참고만 해두셔도 좋을것같아요~
https://developer.mozilla.org/en-US/docs/Web/CSS/appearance
물론 해주신 방법대로 해도 문제될건 없습니다!
| import Link from "next/link"; | ||
|
|
||
| export default function BestItemCard({ content, updatedAt, likeCount, writer, image, title }: Article) { | ||
| export default function BestItemCard({ id, content, updatedAt, likeCount, writer, image, title }: Article) { |
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.
ArticleItemCard 이랑 차이점이 size밖에 없는것같은데, 이런 부분은 아에 props로 size를 지정해주면 오히려 관리하기가 좀더 좋을것같아요ㅎㅎ!
|
|
||
| export default function BestItemCard({ content, updatedAt, likeCount, writer, image, title }: Article) { | ||
| export default function BestItemCard({ id, content, updatedAt, likeCount, writer, image, title }: Article) { | ||
| const formattedDate = formatDate(updatedAt); |
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.
큰건 아닌데, 이런 상수성 변수들은 차라리 useMemo 로 감싸두면 좋긴 해요ㅎㅎ!
useMemo랑 useCallback 쓰는 습관을 들이면 좋긴 한데, React 버전 올라가면서 없어질꺼라는 얘기도 나오긴 하더라구요;;
| const ACCESS_TOKEN_KEY = "accessToken"; | ||
| const REFRESH_TOKEN_KEY = "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.
자잘하긴 하지만...ㅎㅎ;;
보통 상수값은 대문자로 값도 넣어주긴 해요!
const ACCESS_TOKEN_KEY = "ACCESS_TOKEN_KEY" as const;
const REFRESH_TOKEN_KEY = "REFRESH_TOKEN_KEY" as const;| export function saveTokens(accessToken: string, refreshToken: string) { | ||
| localStorage.setItem(ACCESS_TOKEN_KEY, accessToken); | ||
| localStorage.setItem(REFRESH_TOKEN_KEY, refreshToken); | ||
| } | ||
|
|
||
| export function getAccessToken() { | ||
| return typeof window !== "undefined" ? localStorage.getItem(ACCESS_TOKEN_KEY) : null; | ||
| } | ||
|
|
||
| export function getRefreshToken() { | ||
| return typeof window !== "undefined" ? localStorage.getItem(REFRESH_TOKEN_KEY) : null; | ||
| } | ||
|
|
||
| export function clearTokens() { | ||
| localStorage.removeItem(ACCESS_TOKEN_KEY); | ||
| localStorage.removeItem(REFRESH_TOKEN_KEY); | ||
| } |
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.
깔끔하고 좋네요ㅎㅎ!
clearToken 도 원하는 키값에 대해서만 저렇게 수정하면 좋아요
혹시나 싶어서... 전체 다 없애고 싶으시면 아래처럼 하시면 되요!
localStorage.clear()물론 이거보단 위에서 해주신 각각 키값으로 삭제하는게 좀더 좋긴 해요!
원하지 않게 사이드이펙트가 생길 수 있어서요ㅠ
| interface PostPayload { | ||
| title: string; | ||
| content: string; | ||
| image?: File | null; | ||
| } |
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.ts 로 분리시켜주면 조금더 좋을것같긴 하네요ㅎㅎ!
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.
너무 좋긴 한데, API 별로 Service가 나오면 관리하기가 조금 어려워지지 않을까요?
물론 지금 하신것도 잘못된건 아니긴 합니다만 관리하는 측면에서는 어느정도 묶어주는게 좀더 낫지않나 싶어서요ㅎㅎ
| export default function SignupPage() { | ||
| const router = useRouter(); | ||
|
|
||
| const handleSignup = async (formData: { |
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.
formData 라고 하니까 뭔가 진짜 FormData 인것같긴 하네여ㅎㅎ;;
진짜 FormData 라고 하면 formData 라고 이름을 지어도 괜찮은데, 단순 object인데 formData 라고 하면 혼동을 줄 수 있으니 다른 이름을 쓰는건 어떨까요?
저는 보통 API 요청을 보내기 위한 객체는 request 나 좀더 구체적으로 signupRequest 같은이름을 쓰긴 해요ㅎㅎ!
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.
아 그리고 차라리 api 의 타입을 정의할때 같이 정의해버려서 해당 타입을 쓰면 편해요ㅎㅎ!
api.ts
export const signup = async(request: SignupRequest): Promise<SignupResponse> => {
...
}이런식으루요ㅎㅎ!
어차피 저렇게 타입을 정의해두면 쓰게되더라구요
| const { accessToken, refreshToken } = await login(formData); | ||
| saveTokens(accessToken, 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.
물론 여기서 saveToken을 해주어도 되긴하지만 login api의 특징을 생각해보았을땐 아에 saveToken을 login 함수 안쪽에서 해주는것도 좋을것같은데 어떠신가요??
왜냐하면 로그인을 호출해서 성공했다는건 token을 storage 에 저장하는거니까요ㅎㅎ!
하나의 동작으로 보는거죠~
| if (typeof id !== "string") { | ||
| return { | ||
| notFound: 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.
오 이런 예외처리 너무 좋습니다!
외부에서 들어오는 input 에 대해서는 항상 타입과 값을 검증하는 validation 처리를 해주세요!
그런데 위 경우에선 딱히 의미가 없긴 해요ㅠ
왜냐하면 어차피 context.params에 있는 값은 다 string 일꺼거든요ㅠ
그래서 이렇게 검증하는것보단
if (!Number(id)) {
return {
notFound: true
}
}가 좀더 유용할것같아요!
참고로 NaN은 false 랑 동일해요ㅎㅎ! 정확히는 falsy...
요구사항
기본
상품 등록 페이지
상품 상세 페이지
심화
상품 등록 페이지
주요 변경사항
스크린샷
멘토에게