Skip to content
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

[김민재] Sprint12 #723

Open
wants to merge 3 commits into
base: Next.js-김민재
Choose a base branch
from

Conversation

PixeIDark
Copy link
Collaborator

@PixeIDark PixeIDark commented Jul 19, 2024

요구사항

기본

  • [x]
  • []
  • []

심화

  • [x]
  • []

주요 변경사항

  • https://neview12.netlify.app/
  • 리액트 쿼리에 상태 관리하는 것만 구현했습니다 items폴더를 중점적으로 봐주시면 좋을 거 같아요.

스크린샷

image

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@PixeIDark PixeIDark self-assigned this Jul 19, 2024
@PixeIDark PixeIDark added the 미완성🫠 죄송합니다.. label Jul 19, 2024
@PixeIDark PixeIDark changed the base branch from main to Next.js-김민재 July 19, 2024 09:57
Copy link
Collaborator

@endmoseung endmoseung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다!
꾸준히 과제를 제출해주시는점이 너무 멋있으세요.
10개정도 리뷰 남겼으니 확인하시고 더 궁금하신점 있으면 편하게 말씀해주세요:)

Comment on lines +5 to +17
remotePatterns: [
{
protocol: "https",
hostname: "sprint-fe-project.s3.ap-northeast-2.amazonaws.com",
port: "",
pathname: "**",
},
{
protocol: "https",
hostname: "example.com",
port: "",
pathname: "**",
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시나 이미지를 개발할 떄 이 값을 추가해줘야 하는 원인이 궁금하시면 nextJSImage를 읽어보면 좋아요!

@@ -9,6 +9,8 @@
"lint": "next lint"
},
"dependencies": {
"@tanstack/react-query": "^5.51.4",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react-query가 5버젼부터 정말 많이 바꼈는데 4버젼과 비교되는 부분을 한번 봐도 재밌어요!

Comment on lines +9 to +14
return (
<div>
{list.map((item, index) => (
<ProductCard item={item} key={item.id} />
))}
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key값을 id로 한 것 좋아요!
다만 추후에 비슷한 성질의 데이터를 map할때 모두 id로 돌리면 key값 중복이 되있으므로 리터럴 문자로 id값과 uniuqe한 string의 조합으로 해주시면 좋아요!

Comment on lines +9 to +14
return (
<div>
{list.map((item, index) => (
<ProductCard item={item} key={item.id} index={index} />
))}
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

semantic

Suggested change
return (
<div>
{list.map((item, index) => (
<ProductCard item={item} key={item.id} index={index} />
))}
</div>
return (
<ul>
{list.map((item, index) => (
<ProductCard item={item} key={item.id} index={index} />
))}
</ul>

시멘틱 태그를 준수하기 위해 ul태그로 추후 바꿔주세요:)

Comment on lines +7 to +8
// index를 프롭으로 받게되면서 범용성이 많이 떨어짐 전체 상품에서도 쓸만했는데..
function ProductCard({ item, index }: { item: Product; index?: number }) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 컴포넌트의 props를 위에 따로 타입으로 빼줘도 좋겠네요!

Suggested change
// index를 프롭으로 받게되면서 범용성이 많이 떨어짐 전체 상품에서도 쓸만했는데..
function ProductCard({ item, index }: { item: Product; index?: number }) {
interface ProductCardProps {
item: Product;
index?: number
}
function ProductCard({ item, index }: ProductCardProps) {

display: {
base: typeof index === "number" && index > 0 ? "none" : "block",
md: typeof index === "number" && index > 1 ? "none" : "block",
xl: "block",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index > 0값을 따로 변수로 빼 imageSize에도 같이 적용해주면 좋겠네요.
자바스크립트의 이상한 동작때문에 추후 빈스트링이이나 빈 배열은 Truthy한 값이라 추후 런타임 에러를 발생할 확률이 높아져요

@@ -1,3 +1,4 @@
import { useEffect, useState } from "react";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋네요!
다만 화면의 반응형은 주로 클라이언트에서 적용되니 use client를 붙여주면 추후 런타임에러가 발생하지 않을 것 같아요.


export default function App({ Component, pageProps }: AppProps) {
return <Component {...pageProps} />;
const [queryClient] = React.useState(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useState를 import해서 쓰셔도 되고 React객체에 직접 접근해서 쓰셔도 되는데 컨벤션 통일을 위해서 하나로 부탁드릴게요.
만약 사용하시지 않는다면 리액트 16.8버젼 이상부터는 따로 React를 import 하지 않아도 동작해요

Comment on lines +24 to +26
const bestProducts = await getProducts(bestProductsOption);
const allProducts = await getProducts(allProductsOption);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서버사이드의 용도가 이렇게 값들을 조합해서 클라에 내려주는 용도 이기도 해요!🌞

Comment on lines +38 to +45
queryKey: ["bestProductsData"],
queryFn: () => getProducts({ orderBy: "favorite", pageSize: 3, page: 1 }),
initialData: bestProducts,
});

const { data: allProductsData } = useQuery({
queryKey: ["allProductsData"],
queryFn: () => getProducts({ orderBy: "recent", pageSize: 10, page: 1 }),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queryKey를 세분화해서ㅏ orderBy, pageSize, page의 값들도 key로 관리하면 추후 데이터 관리에 이점이 있어요~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
미완성🫠 죄송합니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants