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

[최성락] Sprint7 #260

Conversation

rak517
Copy link
Collaborator

@rak517 rak517 commented Dec 7, 2024

요구사항

기본

상품 상세

  • 상품 상세 페이지 주소는 "/items/{productId}" 입니다.
  • response 로 받은 아래의 데이터로 화면을 구현합니다.
  • 목록으로 돌아가기 버튼을 클릭하면 중고마켓 페이지 주소인 "/items" 으로 이동합니다

상품 문의 댓글

  • 문의하기에 내용을 입력하면 등록 버튼의 색상은 "3692FF"로 변합니다.
  • response 로 받은 아래의 데이터로 화면을 구현합니다

심화

  • 모든 버튼에 자유롭게 Hover효과를 적용하세요.

스크린샷

screencapture-localhost-3000-items-226-2024-12-07-22_42_07

멘토에게

  • 코드 리뷰에서 말씀해주신 error 클래스를 상속받아 HTTP 오류에 대한 메시지를 던져주는 custom error exception class 만들어 봤습니다.
  • 예전에는 UI 컴포넌트 안에서 데이터 관련 함수를 처리하다가 이번에는 좀 더 편할거 같아서 상품 상세 페이지에서 UI에서 사용하는 데이터 관련 함수들을 빼서 커스텀 훅으로 만들었는데 이렇게 사용해도 될까요?
  • 1일 전, 2개월 전 처럼 날짜 포맷하는건 GPT 참조하여 만들었습니다.
  • 페이지 작업할때 분리를 해놓고 작업했어야 하는데 모두 작업하고 분리해서 구조가 조금 복잡합니다ㅜ
  • 급하게 작업하느라 className 등 가독성이 조금 떨어집니다 ㅠㅠ

@rak517 rak517 added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Dec 9, 2024
@1005hoon 1005hoon self-requested a review December 12, 2024 13:07
Copy link
Collaborator

@1005hoon 1005hoon left a comment

Choose a reason for hiding this comment

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

성락님! 코드 리뷰 드리던 내용들이 많이 반영이 되고 있는게 보이네요 ㅎㅎ
이제 조금만 더 해보면 완성도 높은 작업물이 나올듯 싶습니다.
일단 전반적으로 다 너무 잘 하셨고요, 피드백 드린 부분들만 한번 반영해서 업데이트 해보면 좋을듯 싶어요. 고생 많으셨습니다. 팀 프로젝트가 기대되네요!

Comment on lines +23 to +29
if (error instanceof HttpException) {
throw error;
} else {
console.error("네트워크 오류", error);
throw new Error("네트워크 오류가 발생했습니다. 잠시 후 다시 시도해주세요.");
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

너무 좋습니다. 이렇게 서버단에서 오류던져준건 그에 맞춰서 전파해주고,
네트워크 또는 알수없는 오류의 경우 다르게 처리해서 던져줌으로 훨씬 더 ui쪽에서 디버깅하기가 수월해질거에요

import ItemCard from "../ui/Item/ItemCard";
import "./AllProductsSection.css";

const getPageSize = () => {
const width = window.innerWidth;
const getPageLimit = (width) => {
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 +28 to +32
if (error instanceof HttpException) {
setError(error.message);
} else {
setError("알 수 없는 오류가 발생했습니다.");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

사실 이미 fetchproduct에서 던져지는 에러 문구 핸들링은 다 하고 있잖아요
그래서 바로 setError(error.message)로 통합해서 처리해도 괜찮아보여요

Comment on lines +23 to +25
const getProducts = async (limit, sort) => {
try {
const { list } = await fetchProducts(sort, limit);
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 함수들처렴 인자가 여러개 들어가기 시작한다면, 인자를 객체로 전달받는 형태도 고려해보면 좋습니다

} else {
setError("알 수 없는 오류가 발생했습니다.");
}
}
};

const handleResize = useCallback(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

사실 콜백으로 감싸서 재평가 자체는 안되도록 할 수 있지만 실질적으로 onResizeEvent가 와르르 발생할때에 대한 퍼포먼스 핸들링은 어려울거에요.

따라서 쓰로틀링이나 디바운싱을 활용해보면 좋겠는데요

// utils.js
function debounce(callback, delay = 300) {
  let timeoutId = null;
  return (...args) => {
    if (timeoutId) {
      clearTimeout(timeoutId);
    }
    timeoutId = setTimeout(() => {
      callback(...args);
    }, delay);
  };
}

로 구현해두고

useEffect(() => {
  const handleResize = debounce(() => {
    const newPageSize = getPageLimit(window.innerWidth);
    if (newPageSize !== pageSize) {
      setPageSize(newPageSize);
    }
  }, 300); // 300ms 지연 시간 설정

  window.addEventListener('resize', handleResize);

  return () => {
    window.removeEventListener('resize', handleResize);
  };
}, [pageSize]);

이런식으로 활용해보면 어떨까 합니다

Comment on lines +8 to +13
const getRelativeTime = (createdAt, updatedAt) => {
if (updatedAt && updatedAt !== createdAt) {
return formatRelativeTime(updatedAt);
}
return formatRelativeTime(createdAt);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

하나 의견있는데요, 지금 이 로직의 경우

  1. 생성일과 수정일이 동일하다면, 생성일 기준으로 날짜 파싱
  2. 생성일과 수정일이 다르다면 수정일 기준으로 날짜 파싱
    을 하고 있잖아요?

근데 결국 위 두 로직 다 결국 수정일을 기준으로 날짜 파싱해도 동일한 결과가 나오는 것으로 생각됩니다.

애초에 디비 설계상 데이터가 최초 생성되면 수정일도 해당일과 동일하게 생성이 될거고,
특정 엔티티를 수정하면 수정일만 변경이 되다보니깐요.

만약 코드잇측에서 디비 설계를 좀 요상하게 해서 최초 생성시 수정시간이 등록되지 않는다면 지금처럼 두어도 무난하지만 그게 아니라면 updatedAt 기준만 활용해서 로직 파싱해도 될것 같습니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 아마 이 코드가 성락님이 여쭤보신 ui가 아닌곳에서 필요한 값을 꺼내오는 부분이라고 생각되어요.

이건 사실 개인 취향에 따라 달려있는기능이긴 한데요, 날짜 데이터의 경우, 되도록이면 UI 콤포넌트에게 날짜형데이터는 날짜형태로 전달을 해주고, 환경에 따라서 UI로 파싱하는건 UI 콤포넌트에서 util function을 활용해 풀어주는걸 선호하고 있어요!

왜냐면 나중에 콤포넌트 관리할때, 날짜를 보여주는 부분을 수정하고자 한다면 당연히 UI 콤포넌트로 먼저 이동하게 될텐데, 거기에서 이미 다 형처리가 완료된 데이터를 prop으로 전달받는다면 UI 관리 책임이 조금 어긋난다고 생각하기 때문이에요.

이거는 한번 성락님 취향에 맞춰 탐색해보시면 어떨까요?

Comment on lines +27 to +35
{tags && tags.length > 0 ? (
tags.map((tag) => (
<li key={tag} className="item-detail-tag">
#{tag}
</li>
))
) : (
<li></li>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

리스트가 없으면 아무것도 렌더링 안해주면 안되나용?

Suggested change
{tags && tags.length > 0 ? (
tags.map((tag) => (
<li key={tag} className="item-detail-tag">
#{tag}
</li>
))
) : (
<li></li>
)}
{tags && tags.map((tag) => (
<li key={tag} className="item-detail-tag">
#{tag}
</li>
)
)}

Comment on lines +7 to +13
const formatDate = (dateString) => {
const date = new Date(dateString);
const year = date.getFullYear();
const month = String(date.getMonth() + 1).padStart(2, "0");
const day = String(date.getDate()).padStart(2, "0");
return `${year} ${month}.${day}`;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

date util파일에 옮겨볼까요?

Comment on lines +6 to +7
const [item, setItem] = useState(null);
const [comments, setComments] = useState([]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

댓글과 product detail을 분리해서 훅으로 두면 어떨까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

사용할떄 다음과 같이 해보면 좋지 않을까 싶어요

function ProductDetailPage.jsx() {
  const params = useParams()
  const productId = params.id;

  const {data: productDetail, isLoading: isProductLoading, error: productError} = useProductDetail(productId)
  const { data: comments, isLoading: commentsLoading, error: commentsError } = useCommentForProduct(productId)
...
}

Comment on lines +18 to +21
<div className="item-detail-page">
<Header />
<p>로딩 중...</p>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분들이 계속 반복되고 있죠?
그럼 렌더링 하는 곳에서 이렇게 하는게 더 수월할 수 있습니다

return <div className='item-detail-page'>  
  <Header />
  {loading && 로딩중 }
  {error && 에러 콤포넌트 }
  { !loading && !error && data && 데이터 콤포넌트 }
</div>

@1005hoon 1005hoon merged commit 7ead2c4 into codeit-bootcamp-frontend:React-최성락 Dec 13, 2024
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.

3 participants