-
Notifications
You must be signed in to change notification settings - Fork 42
[박연희] sprint5 #167
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
base: React-박연희
Are you sure you want to change the base?
The head ref may contain hidden characters: "React-\uBC15\uC5F0\uD76C-sprint5"
[박연희] sprint5 #167
Conversation
[Fix] delete merged branch github action
…-sprint1 [박연희] sprint1
…-sprint2 [박연희] sprint2
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.
수고하셨습니다~!
기본기가 탄탄하시네요 :)
주요 리뷰 포인트
- 유지보수를 고려한 개발
- 리턴문 가독성 높이기
- 디렉토리 구조
return body; | ||
} | ||
|
||
export function useProductData({ page, pageSize, isPageinated = true }) { |
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.
isPageinated -> isPaginated 오타났네요!
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.
https://marketplace.cursorapi.com/items?itemName=streetsidesoftware.code-spell-checker
code spell checker extension 설치하셔서 오타나 틀린 문법 교정 도움받아보세요! :)
useEffect(() => { | ||
if (!pageSize) return; // pageSize가 없으면 API를 아예 호출하지 않게 차단한다. | ||
const fetchData = async () => { | ||
try { | ||
const query = isPageinated ? { page, pageSize } : { pageSize }; | ||
const data = await getProducts(query); | ||
setProducts(data.list); | ||
console.log("getProducts 결과:", data.list); | ||
|
||
if (isPageinated) { | ||
if (data.totalPages) { | ||
setTotalPages(data.totalPages); | ||
} else if (data.totalCount) { | ||
setTotalPages(Math.ceil(data.totalCount / pageSize)); | ||
} | ||
} | ||
} catch (error) { | ||
console.error("상품 불러오기 실패", error); | ||
} | ||
}; | ||
|
||
fetchData(); | ||
}, [page, pageSize, isPageinated]); |
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.
페이지네이션 로직과 data fetching 로직을 분리해주면 코드 중복을 줄이고 재사용에 도움이 되겠죠?
pagination만 처리하는 커스텀 훅을 따로 만들고,
해당 훅에서 이 훅을 재사용해보는 구조로 바꿔봅시다 :)
예시를 보여드릴게요!
import { useState, useEffect } from 'react';
export function usePagination({
fetchData,
pageSize,
initialPage = 1,
isEnabled = true
}) {
const [currentPage, setCurrentPage] = useState(initialPage);
const [totalPages, setTotalPages] = useState(1);
const [data, setData] = useState([]);
const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState(null);
useEffect(() => {
if (!isEnabled || !pageSize) return;
const loadData = async () => {
setIsLoading(true);
setError(null);
try {
const result = await fetchData({ page: currentPage, pageSize });
setData(result.list);
if (result.totalPages) {
setTotalPages(result.totalPages);
} else if (result.totalCount) {
setTotalPages(Math.ceil(result.totalCount / pageSize));
}
} catch (err) {
setError(err);
console.error('데이터 로딩 실패:', err);
} finally {
setIsLoading(false);
}
};
loadData();
}, [currentPage, pageSize, isEnabled, fetchData]);
const goToPage = (page) => {
if (page >= 1 && page <= totalPages) {
setCurrentPage(page);
}
};
const nextPage = () => {
if (currentPage < totalPages) {
setCurrentPage(prev => prev + 1);
}
};
const prevPage = () => {
if (currentPage > 1) {
setCurrentPage(prev => prev - 1);
}
};
return {
currentPage,
totalPages,
data,
isLoading,
error,
goToPage,
nextPage,
prevPage,
setCurrentPage
};
}
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.
이제 기존의 useProductData 훅을 수정해서 새로 만든 usePagination 훅을 사용하도록 변경해볼게요.
export function useProductData({ pageSize, isPageinated = true }) {
const {
currentPage,
totalPages,
data: products,
isLoading,
error,
goToPage,
nextPage,
prevPage,
} = usePagination({
fetchData: getProducts,
pageSize,
isEnabled: isPageinated,
});
return {
products,
currentPage,
totalPages,
isLoading,
error,
goToPage,
nextPage,
prevPage,
};
}
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.
이렇게 구조를 바꿔주면, 이런 장점들이 생길 수 있습니다.
- 재사용성: usePagination 훅은 어떤 종류의 데이터 페칭 함수와도 함께 사용될 수 있고, 이때 페이지네이션 로직을 재사용할 수 있습니다.
- 일관성 향상 및 기능 확장: 일관적이고 명확한 방식으로 로딩 상태를 관리하고, 에러를 처리하고, 페이지 이동 메서드를 관리하고, 현재 페이지 상태를 관리할 수 있습니다. 또한 페이지네이션 관련 기능을 수정하고 확장하기도 용이해집니다.
- 관심사 분리: 페이지네이션 로직이 데이터 페칭 로직과 분리되어 있어 각각의 책임이 명확해집니다.
// api 불러오기, 20개 불러온 뒤 반응형에 따라 자름 | ||
const { products } = useProductData({ pageSize: 20, isPageinated: false }); | ||
|
||
const sortedItems = [...products].sort((a, b) => b.favoriteCount - a.favoriteCount).slice(0, visibleCount); |
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.
스프레드 연산자로 복사하신점 의도하신걸까요? 자바스크립트 공부 잘하셨네요 👍
NIT: 이미 알고계실수도 있지만 배경 설명을 좀 드려보자면,
sort() 메서드는 원본 배열을 직접 수정(mutate)하는 메서드라서 리액트의 상태 관리 원칙상 상태를 직접 수정하는 것은 피하는게 좋습니다. 따라서 원본 배열을 보존하기 위해 스프레드 연산자로 복사하는 것이 맞아요! 잘하셨어요 👍
왜 리액트를 사용할때 상태의 불변성을 유지하는게 좋은지에 대해서 더 찾아보시면 좋을것같네요 :)
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.
이 컴포넌트에서도, 아까 페이지네이션 로직을 따로 분리한 커스텀훅을 재사용해주세요 :)
{sortedItems.map((product) => ( | ||
<li key={product.id}> | ||
<div className="product__image"> | ||
{!product.images[0] || imgLoadStatus[product.id] === "error" ? ( |
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.
리턴문은 최대한 깔끔히 유지할수있도록 습관을 들여보시는게 좋아요.
인라인으로 조건 평가식을 쓰고있어서 가독성이 떨어지네요!
상수로 만들어주면 어떨까요?
const isError = !product.images[0] || imgLoadStatus[product.id] === "error";
이렇게 상수를 만들어주고 리턴문에서는 해당 상수를 사용해주면, 리턴문에 작성한 주석도 없앨 수 있어요! :)
const [sortKey, setSortKey] = useState("updatedAt"); | ||
const [page, setPage] = useState(1); //현재 페이지 | ||
const [pageSize, setPageSize] = useState(() => pageSizebyScreenWidth(window.innerWidth).all); //상품 개수 | ||
|
||
// api 불러오기 | ||
const { products, totalPages } = useProductData({ page, pageSize, isPageinated: true }); |
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.
지금과 같이 프로덕트 리스트뿐만 아니라 페이지네이션 정렬이 필요한 모든 컴포넌트에서 해당 코드가 반복될테니, 위에서 코멘트드린것처럼 재사용을 위해 커스텀 훅으로 로직을 분리해주시면 좋겠죠? :)
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.
이건 컴포넌트가 아닌 함수가 아닌가요? components 폴더에 들어가있으면 안되고, 확장자도 바꿔주는게 좋을것같아요.
아래 아티클 참고해보시고, 연희님이 편한 구조로 바꿔보세요! :)
질문에 대한 답변
뭐든 천천히 하나씩 시도해보시고 경험을 누적하시면 처음 시도할때보다 훨씬 수월해집니다! 컴포넌트를 쪼개는 방법은 구현 의도에 따라 정말 여러가지 기준이 있을수있어서 딱 정해서 말씀드리기 어렵네요. 우선 리턴문 가독성을 생각했을때 3 depth 이상으로 들어가는것같으면 컴포넌트로 분리해주시는게 좋고 |
요구사항
기본
심화
주요 변경사항
멘토에게