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 #737

Conversation

JiminN2
Copy link
Collaborator

@JiminN2 JiminN2 commented Jul 19, 2024

요구사항

기본

중고마켓
‘상품 등록하기’ 버튼을 누르면 “/additem” 로 이동합니다.
각 상품 클릭 시 상품 상세 페이지로 이동합니다.
상품 상세 페이지 주소는 “/items/{productId}” 입니다.
상품 상세
내가 등록한 상품일 경우 상품 수정, 삭제가 가능합니다.
문의하기 input창에 값을 입력 후 ‘등록’ 버튼을 누르면 댓글이 등록됩니다.
내가 등록한 댓글은 수정, 삭제가 가능합니다.
상품 등록
이미지를 제외하고 input 에 모든 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.
활성화된 ‘등록' 버튼을 누르면 상품 등록이 완료됩니다.
등록이 완료되면 해당 상품 상세 페이지로 이동합니다.

심화

  • [x]
  • []

주요 변경사항

스크린샷

image

멘토에게

  • useQuery 로 데이터 불러오는 것을 처음 시도해봤습니다
  • 중고마켓 페이지만 했습니다
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@JiminN2 JiminN2 requested a review from addiescode-sj July 19, 2024 14:22
Copy link
Collaborator

@addiescode-sj addiescode-sj left a comment

Choose a reason for hiding this comment

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

지민님, 코드 작성 쉽게쉽게 잘 하시네요 ~ :)
전반적으로 사용되는 코드들이 컴포넌트 내부에서 flat하게 모두 꺼내져있는 인상을 받았어요.
재사용할수있는 로직은 custom hook으로 분리하고, 뎁스가 깊어보이는 jsx 구문은 컴포넌트로 분리해보는 습관을 들여보시면 좋을것같네요.

그리고 sprint 12를 제출해주신건데, 작성된 코드만 보자면 Next.js 기반이 아닌것처럼 보이네요..!
Next.js에서 활용하기 좋은 feature (e.g file system based routing, Image 컴포넌트를 사용한 이미지 최적화 등)를 사용해보는 목적으로 리팩토링해보는건 어떨까요?

수고하셨습니다! :)

padding: 20px;
margin: auto 360px;

.SubmitSection {
Copy link
Collaborator

Choose a reason for hiding this comment

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

styled components를 쓰고 있으니, SubmitSection, title, content 모두 개별 컴포넌트를 가지게끔 관리해주시는게 가독성과 유지보수 측면에서 유리하지않을까요?

Comment on lines +47 to +50
<div className="title">
<label htmlFor="title">제목</label>
<input id="title" placeholder="제목을 입력해주세요" />
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 구조가 반복되고있으니, 컴포넌트로 분리해주는게 좋을것같아요 :)

@@ -0,0 +1,27 @@
import React 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.

앗 sprint 미션 12면 next.js 기반이어야할텐데 이 환경에서 router dom을 부가적으로 쓰실 필요는 없어요! ㅎㅎ

console.log(ListPage);

return (
<QueryClientProvider client={queryClient}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

굳굳 :) 리액트 쿼리를 쓰기위해 루트에서 Provider로 쿼리 클라이언트를 잘 내려받아주셨네요 :)

}

// Check if data is defined and has the list property
const products = data?.list || []; // Extract the list of products
Copy link
Collaborator

Choose a reason for hiding this comment

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

리액트 쿼리를 사용한 api 요청도 잘하셨고, 응답결과도 state쓰지않고 잘 써주셨네요!
조건식도 상수화 잘 해주셨고요 👍 👍 지민님 코드 어렵지않게 잘 짜시는것같은데요? ㅎㅎ

Comment on lines +35 to +59
{products.map((product) => (
<div key={product.id}>
{product.images && product.images.length > 0 && (
<div>
<div>
{product.images.map((imageUrl, index) => (
<img
key={index}
src={imageUrl}
alt={`Product ${product.id} image ${index}`}
style={{
width: "282px",
height: "282px",
marginRight: "10px",
}}
/>
))}
</div>
<div>{product.name}</div>
<div>{product.price}원</div>
<div>{product.favoriteCount}</div>
</div>
)}
</div>
))}
Copy link
Collaborator

Choose a reason for hiding this comment

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

map을 이렇게 두번 돌리는것보다는, 컴포넌트 분리를 한 상태에서 한 컴포넌트 body안에서 한번만 map을 돌리게끔 만들어주시는게 코드를 유지보수하는 관점에서 유리할것같아요 :) 어떤 역할을 가진 컴포넌트를 만들어주면 좋을까요~? 리팩토링 주제로 추천드리겠습니다!

Comment on lines +20 to +32
useEffect(() => {
if (data && data.totalCount) {
setTotalPages(Math.ceil(data.totalCount / pageSize));
}
}, [data, pageSize]);

const handlePrevPage = () => {
if (page > 1) setPage((prevPage) => prevPage - 1);
};

const handleNextPage = () => {
if (page < totalPages) setPage((prevPage) => prevPage + 1);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 로직들이 Products 컴포넌트 안에서만 필요할까요?
페이지네이션이 필요한 모든 컴포넌트에서 반복해서 사용될 필요가 있겠죠~?
이런 로직들을 효과적으로 재사용 가능하게 분리해주려면, 커스텀 훅으로 분리해보시는게 좋을것같아요 :)

{product.images && product.images.length > 0 && (
<div>
<div>
{product.images.map((imageUrl, index) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서도 위에 드렸던 코멘트 참고해서, 컴포넌트 분리를 해야겠다는 기준으로 삼아보시면 좋을것같아요 :)

@addiescode-sj addiescode-sj merged commit 3b387b1 into codeit-bootcamp-frontend:React-박지민 Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants