[김인] sprint5 #189
[김인] sprint5 #189
Conversation
…ithub-actions [Fix] delete merged branch github action
addiescode-sj
left a comment
addiescode-sj
left a comment
There was a problem hiding this comment.
수고하셨습니다!
주요 리뷰 포인트
- axios 사용 관련 피드백
- 컴포넌트, 훅 분리 및 설계 관련 피드백
sprint5/src/App.jsx
Outdated
| useEffect(() => { | ||
| const fetchItems = async () => { | ||
| try { | ||
| const response = await axios.get( | ||
| `https://panda-market-api.vercel.app/products?page=${currentPage}&pageSize=10&orderBy=${sortOption}` | ||
| ); | ||
| setItems(response.data.list); | ||
| setTotalPages(response.data.totalPages); | ||
| } catch (error) { | ||
| console.error("상품을 불러오지 못했습니다!", error); | ||
| } | ||
| }; | ||
|
|
||
| fetchItems(); | ||
| }, [sortOption, currentPage]); |
There was a problem hiding this comment.
data fetching 로직을 컴포넌트에서 분리하고, 재사용하기위해 커스텀 훅으로 분리해보면 어떨까요? :)
There was a problem hiding this comment.
그리고 해당 로직이 최상위 레벨인 App 컴포넌트에 있다보니, 모든 컴포넌트가 렌더링될때 의존성을 비교하고 수행될 가능성이 있는데, 불필요한 리렌더링을 방지하려면 해당 data가 필요한 컴포넌트에서 직접적으로 호출하는편이 좋겠죠?
sprint5/src/App.jsx
Outdated
| <main style={{ paddingTop: "80px" }}> | ||
| <section className="product-toolbar"> | ||
| {" "} | ||
| {/* 전체상품과 텍스트 위치 맞추기 위해서 */} | ||
| <div className="toolbar-header"> | ||
| <h2 className="section-title">베스트 상품</h2> | ||
| </div> | ||
| <BestItemCard /> | ||
| </section> | ||
|
|
||
| <section className="product-toolbar"> | ||
| <div className="toolbar-header"> | ||
| <h2 className="section-title">전체 상품</h2> | ||
|
|
||
| <div className="toolbar-actions"> | ||
| <SearchBar /> | ||
| <AddProductButton /> | ||
| <SortDropdown sortOption={sortOption} onChange={setSortOption} /> | ||
| </div> | ||
| </div> | ||
|
|
||
| <AllItemSection items={items} /> | ||
| </section> | ||
|
|
||
| <Pagination | ||
| currentPage={currentPage} | ||
| totalPages={totalPages} | ||
| onPageChange={setCurrentPage} | ||
| /> | ||
| </main> |
There was a problem hiding this comment.
해당 코드들을 Main 컴포넌트로 분리하고 Routes 목록에 포함한다음 index 페이지를 /main 페이지로 항상 이동할수있게끔 해보면 어떨까요?
| useEffect(() => { | ||
| axios | ||
| .get( | ||
| "https://panda-market-api.vercel.app/products?page=1&pageSize=100&orderBy=favorite" | ||
| ) | ||
| .then((response) => { | ||
| const top4 = response.data.list.slice(0, limit); | ||
| setItems(top4); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Base url의 경우 외부 노출을 막기 위해 환경 변수로 관리해주시는게 좋아요.
불필요하게 반복되는 코드들을 줄이기위해 axios instance 방식으로 관리해보면 어떨까요?
sprint5/src/components/ItemCard.jsx
Outdated
| <img | ||
| src={ | ||
| item.images && item.images.length > 0 | ||
| ? item.images[0] | ||
| : "대체이미지주소.png" | ||
| } | ||
| alt={item.name} | ||
| /> |
There was a problem hiding this comment.
이런 코드들도 fallback 이미지를 지정하고싶은거라면 컴포넌트화해두는게 유지보수에 좋겠죠?
There was a problem hiding this comment.
굳굳! 페이지네이션 공통 로직 + UI를 Pagination 컴포넌트로 잘 분리하셨네요.
만약 UI말고 로직만 재사용할 가능성이 있다면 로직만 따로 분리하기위해 커스텀 훅을 사용하셔도 좋겠죠?
|
PR 수정사항 적용했습니다! <작업내용>
항상 좋은 피드백 감사드립니다! |
4 participants
요구사항
기본
심화
주요 변경사항
스크린샷
<i
2" />
멘토에게