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

Conversation

park521
Copy link
Collaborator

@park521 park521 commented Dec 8, 2024

요구사항

기본

  • [x]
  • []
  • []

심화

  • [x]
  • []

주요 변경사항

미완성 파일 제출합니다.

스크린샷

image

멘토에게

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

@park521 park521 requested a review from devToram December 8, 2024 14:33
@park521 park521 self-assigned this Dec 8, 2024
@park521 park521 added the 순한맛🐑 마음이 많이 여립니다.. label Dec 8, 2024
Copy link
Collaborator

@devToram devToram 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 +12 to +16
} else if (width < 1280) {
return 6;
} else {
return 10;
}
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
} else if (width < 1280) {
return 6;
} else {
return 10;
}
}
if (width < 1280) {
return 6;
}
return 10;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

여러 군데서 쓰이는 함수라서 util 화 시켜도 좋을 거 같아요~

Comment on lines +25 to +26
fetchItemData();
fetchCommentData();
Copy link
Collaborator

Choose a reason for hiding this comment

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

useEffect 내부에서만 사용하는 함수인 경우 useEffect 내부에서 선언하는 걸 추천드려요!


function DetailPage() {
const { itemSlug } = useParams();
const [item, setItem] = useState(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

item 을 구조분해 할당해서 사용하는 것도 좋을 거 같아요!

import { ReactComponent as RightArrow } from "../assets/arrow_right.svg";

const PaginationBar = ({ totalPageNum, activePageNum, onPageChange }) => {
const maxVisiblePages = 5;
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 +9 to +14
if (totalPageNum <= maxVisiblePages) {
startPage = 1;
} else {
startPage = Math.max(activePageNum - Math.floor(maxVisiblePages / 2), 1);
startPage = Math.min(startPage, totalPageNum - maxVisiblePages + 1);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분에서 startPage 값이 정해지기 때문에 let 대신 const 쓰도록 바꿀 수 있을 거 같아요!

return (
<div className={styles.tagsDisplaySection}>
{tags.map((tag, index) => (
<div className={styles.tag} key={`tag-display-${index}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

단순히 index 를 key 로 주는 건 안주는 것과 동일해요! (실제로 key 안주면 react 내부에서 처리하는 방식)

@devToram devToram merged commit e7b84b7 into codeit-bootcamp-frontend:React-박준환 Dec 9, 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.

2 participants