-
Notifications
You must be signed in to change notification settings - Fork 31
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
[오하영] Sprint 5 #129
The head ref may contain hidden characters: "react-\uC624\uD558\uC601"
[오하영] Sprint 5 #129
Conversation
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.
[P0]
리액트는 JSX 문법을 사용하기에 모든 리액트 컴포넌트 파일의 확장자를 .jsx로 변경하셔야 합니다.
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.
제 코드리뷰 앞에 붙어있는 태그들의 의미는 아래와 같습니다.
- P0 : 꼭 반영해 주세요 (Request Changes) - 이슈가 발생하거나 취약점이 발견되는 케이스 등
- P1 : 반영을 적극적으로 고려해 주시면 좋을 것 같아요 (Comment)
- P2 : 이런 방법도 있을 것 같아요~ 등의 사소한 의견입니다 (Chore)
코드가 전반적으로 깔끔한 것 같습니다. 작성하시느라 고생하셨습니다!
처음에 남겨둔 확장자 관련 코멘트는 꼭 고쳐주셔야 합니다!
const handleLoad = async () => { | ||
const data = await getProducts(1, 250); | ||
setItems(data.list); | ||
}; |
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.
[P0]
async await 를 통해 API를 호출하는 경우 try catch 문을 통해 에러 핸들링을 해주는 것이 좋습니다.
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.
확인해보니 src/api/api.js에서 try catch를 처리하셨네요.
이런 경우에는 API 코드내에 try catch 문이 있는 경우에는 해당 catch 부분에서 console 뿐만이 아닌 에러 토스트등을 통한 사용자에게 API 에러가 났음을 알리고 재시도할 수 있는 환경을 만들어주면 좋습니다.
const updatePageSize = () => { | ||
if (window.matchMedia("(max-width: 743px)").matches) { | ||
setPageSize(4); | ||
} else if (window.matchMedia("(max-width: 1199px)").matches) { | ||
setPageSize(6); | ||
} else { | ||
setPageSize(10); | ||
} | ||
}; |
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.
[P2]
미디어 쿼리를 위한 상수 혹은 order를 위한 값 "recent"와 같은 값들은 별도의 constant 파일이나 코드 상단에 선언해서 재사용하는 것이 좋습니다.
유지보수 및 추후 수정이 필요한 경우 한번에 변경할 수 있어 용이합니다.
function ProductListItem({ item }) { | ||
return ( | ||
<div> | ||
<img className="img" src={item.images} alt={item.name} /> | ||
<div className="description"> | ||
<div className="name">{item.name}</div> | ||
<div className="price">{item.price}원</div> | ||
<div className="favoriteCount"> | ||
<img src={unheartIcon} /> | ||
{item.favoriteCount} | ||
</div> | ||
</div> | ||
</div> | ||
); | ||
} |
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.
[P1]
별도의 컴포넌트 파일로 분리해주는 것이 좋습니다!
Compound Component 패턴이 아닌 이상 별도의 파일을 통해 관리하는 것이 좋습니다.
https://patterns-dev-kr.github.io/design-patterns/compound-pattern/
return ( | ||
<div className="pagination"> | ||
<button type="button" className="prevBtn" onClick={handlePrev}> | ||
<img src={arrowLeft} /> |
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.
[P0]
alt 속성을 추가해주시는 것이 좋습니다!
if (window.matchMedia("(max-width: 743px)").matches) { | ||
setPageSize(1); | ||
} else if (window.matchMedia("(max-width: 1199px)").matches) { |
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.
app.js에 파일에 남겨두었듯이 미디어 쿼리 상수를 constant 파일로 관리하면 가져와서 쓸 수 있습니다.
<main className="bestProduct"> | ||
<div className="title">베스트 상품</div> | ||
<div className="list"> | ||
{bestItems.map((item) => ( |
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.
[P1]
bestItems의 배열 길이가 0일때를 생각해서 예외처리를 해주면 좋을 것 같습니다. 항상 배열 값들은 예외처리를 생각하고 empty view를 구현하는 것이 좋습니다.
if (page) url.searchParams.append("page", page); | ||
if (pageSize) url.searchParams.append("pageSize", pageSize); | ||
if (keyword) url.searchParams.append("keyword", keyword); | ||
url.searchParams.append("orderBy", orderBy); | ||
const res = await fetch(url); | ||
if (!res.ok) { | ||
console.error(res.message); | ||
} | ||
const body = await res.json(); | ||
return body; |
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.
[P2]
지극히 개인적인 부분입니다만 1 line으로 구성된 if 문들과 각 구문들이 줄바꿈이 잘 되어있지 않아 코드 가독성이 떨어지는 것 같습니다.
lint 나 prettier등을 통해 자동으로 포맷팅되도록 해보면 좋을 것 같습니다!
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.
추가적으로 create-react-app이 공식적으로 지원이 중단될 예정입니다.
리액트 프로젝트를 시작하실 때 vite 혹은 다른 프레임워크를 사용해서 시작하는 방법에 대해서도 찾아보시면 좋을 것 같아요!
관련 글 https://react.dev/blog/2025/02/14/sunsetting-create-react-app
css를 적용하면서 이미지가 없는 상품 데이터들이 포함된 리스트는 자꾸 비율이 이상하게 돼서 max, min 높이와 넓이를 모두 적용시켰는데 저런 상황에서도 일반적으로 width, height를 적용할 수 있는 좋은 방법이 있을까요? 디자인 패턴을 공부해야 할 것 같은데 컴포넌트를 저정도로만 나눠도 되는지 아니면 버튼이나 검색창 등 더 세분화해서 컴포넌트를 분리하는 것이 좋은지 잘 모르겠습니다. 저 같은 경우에는 컴포넌트를 우선 구현한다음 한 컴포넌트에 너무 많은 기능들이 들어가 있거나 코드의 가독성이 떨어지거나 앞으로 추가적인 기능적 요구사항이 들어올 경우에 리팩토링을 통해 컴포넌트를 다듬는 방식으로 개발을 주로 합니다. 페이지네이션을 더 효율적으로 구현할 수 있는 방법이 있는지 궁금합니다. 지금 있는 코드에서 최대한 개선을 해본다면 기본적으로 페이지의 값을 관리하는 하나의 함수로 정리할 수 있을 것 같습니다. 모든 값들이 currentPage에 종속되어있기 때문에 아래와 같이 개선해보고 useMemo, useCallback과 같은 메모이제이션을 통해 성능 개선을 노려볼 수 있습니다. const paginationData = useMemo(() => {
const totalPages = Math.ceil(totalCount / limit);
// 현재 페이지가 속한 그룹 계산
const currentGroup = Math.ceil(currentPage / offset);
const startPage = (currentGroup - 1) * offset + 1;
const endPage = Math.min(startPage + offset - 1, totalPages);
// 페이지 번호 배열 생성
const pages = [];
for (let i = startPage; i <= endPage; i++) {
pages.push(i);
}
return {
totalPages,
startPage,
endPage,
pages,
hasPrev: currentPage > 1,
hasNext: currentPage < totalPages
};
}, [currentPage, totalCount, limit, offset]);
const handlePrev = () => {
if (currentPage > 1) {
setCurrentPage(currentPage - 1);
}
};
const handleNext = () => {
if (currentPage < paginationData.totalPages) {
setCurrentPage(currentPage + 1);
}
}; paginationData로 부터 받은 값들을 적절히 사용하여 컴포넌트를 구성하면 될 것 같습니다. |
요구사항
기본
공통
중고마켓 페이지
심화
공통
중고마켓 페이지
주요 변경사항
스크린샷
멘토에게