-
Notifications
You must be signed in to change notification settings - Fork 39
[박다인] sprint5 #172
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
[박다인] sprint5 #172
The head ref may contain hidden characters: "React-\uBC15\uB2E4\uC778-sprint5"
Conversation
addiescode-sj
left a comment
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.
수고하셨습니다!
첫미션인데 잘 해주셨네요! 조금만 다듬어보면 멋진 결과물 나올거예요 :)
주요 리뷰 포인트
- pagination 업데이트 로직 및 유지보수를 고려한 수정
- DRY 원칙에 따른 코드 중복 제거
| @@ -0,0 +1,12 @@ | |||
| const BASE_URL = "https://panda-market-api.vercel.app"; | |||
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.
BASE_URL와 같이 민감한 정보의 경우 일반적으로 외부 공개를 막기위해 환경변수로 관리한답니다. React의 경우 환경변수 설정시 변수명에 REACT_APP_ 접두어가 꼭 필요하니, 아래 아티클 확인해주시고 환경변수 설정해보세요!
| const [orderBy, setOrderBy] = useState("recent"); | ||
| const [itemList, setItemList] = useState([]); | ||
| const [page, setPage] = useState(1); | ||
| const [pageSize, setPageSize] = useState(getPageSize()); |
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.
useState setter에 window를 참조하고 동기적으로 실행되는 코드를 작성하는 현재 코드는
몇가지의 기술적 배경을 고려한다면 개선하시는게 좋습니다.
다른분 PR에 달아드린 코멘트인데, 참고해보시고 리팩토링해보세요!
| <input | ||
| className="searchInput" | ||
| placeholder="검색할 상품을 입력해주세요" | ||
| /> |
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.
input (text input field)의 경우 여러 페이지에서 UI가 반복될텐데, 공용 컴포넌트로 만들어 재사용해주면 코드 중복이 줄어들겠죠? :)
| <button className="sortIconButton" onClick={toggleDown}> | ||
| <img src={sorticon} alt="정렬버튼" /> | ||
| </button> |
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.
button도 마찬가지! 여러 페이지에서 재사용되는 UI는 공용 컴포넌트로 만들어서 정리하는걸 습관화해봅시다 :)
| <div className="pageNavigationBar"> | ||
| <PageBar | ||
| totalPage={totalPage} | ||
| activePage={page} | ||
| onPageChange={onPageChange} | ||
| /> | ||
| </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.
PageNavigationBar 네이밍이 일반적으로 많이 사용하지 않고, 모호한 이름으로 느껴지네요.
보통 페이지네이션을 위한 UI는 Pagination을 많이 사용하는것같아요.
| const getPageSize = () => { | ||
| const width = window.innerWidth; | ||
| if (width < 768) { | ||
| //mobile | ||
| return 1; | ||
| } else if (width < 1280) { | ||
| //tablet | ||
| return 2; | ||
| } else { | ||
| //desktop | ||
| return 4; | ||
| } | ||
| }; |
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.
이 함수와 pageSize 초기값에 활용하는것까지 바로 위에 있는 AllItemsSection.jsx 에서 작성해줬던것같은데, 코드 중복을 제거하기위해 따로 이 함수부분만 유틸 함수를 쓰거나 state 업데이트까지 고려해 커스텀 훅으로 분리해주는건 어떨까요?
DRY, SOLID 원칙에 대해 공부해보시면 코드를 개선할때 어떤 기준으로 개선하면 좋을지에 대해 도움 받으실수있을거예요 :)
참고
| <button | ||
| className="pageButton" | ||
| disabled={activePage === 1} | ||
| onClick={() => setCurrentGroup((prev) => Math.max(prev - 1, 0))} |
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.
currentGroup을 업데이트할때 이런식으로 조건에 의해 함수를 따로 만들어 페이지 그룹 전환 로직을 명확하게 분리해볼까요?
페이지 그룹이 실제로 전환되려면 prev, next 버튼을 클릭 시 아래 핸들러를 실행해야할것같아요 :)
const handlePrevGroup = () => {
if (currentGroup > 0) {
setCurrentGroup(currentGroup - 1);
onPageChange(startPage - maxVisiblePages);
}
};
const handleNextGroup = () => {
if (currentGroup < maxGroup) {
setCurrentGroup(currentGroup + 1);
onPageChange(startPage + maxVisiblePages);
}
};| disabled={activePage === 1} | ||
| onClick={() => setCurrentGroup((prev) => Math.max(prev - 1, 0))} | ||
| > | ||
| <img src={leftBtn} alt="왼쪽" /> |
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.
그리고 버튼 아이콘의 경우 조건에 따라 색깔을 바꾸는 등의 처리가 부가적으로 필요한데, svg 속성의 fill로 바꿔주실 수 있으니 svg를 컴포넌트화해서 props로 넘겨주는 방식은 어떨까요? 아래 예시처럼요 :)
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.
- ArrowIcon을 컴포넌트화해서, 방향과 컬러에 대한 props 넘겨주기
import React from "react";
const ArrowIcon = ({ color = "#000000", direction = "left" }) => {
return (
<svg
width="24"
height="24"
viewBox="0 0 24 24"
fill="none"
xmlns="http://www.w3.org/2000/svg"
style={{
transform: direction === "right" ? "rotate(180deg)" : "none",
transformOrigin: "center",
}}
>
<path
d="M15 18L9 12L15 6"
stroke={color}
strokeWidth="2"
strokeLinecap="round"
strokeLinejoin="round"
/>
</svg>
);
};
export default ArrowIcon;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.
사용 예시:
<button
className="pageButton"
disabled={currentGroup === 0}
onClick={handlePrevGroup}
>
<ArrowIcon
color={currentGroup === 0 ? "#CCC" : "#000"}
direction="left"
/>
</button>
질문에 대한 답변
Array.from은 이렇게 작동됩니다!
startPage 계산이 현재 로직에선 중요한데, 이게 제때 업데이트 안되는 문제가 있었네요 :)
특정 상태에 따라 버튼의 이름이나 스타일이 바뀌어야하는거면 우선 상태가 올바르게 업데이트 되는지 디버깅해보시고 상태에 따른 조건부 렌더링 혹은 스타일링을 할수 있게 구현해주셔야겠죠? :) orderBy state를 관리하시는것같은데, 해당 state의 value를 추적하고 |
요구사항
기본
중고마켓
중고마켓 반응형
심화
주요 변경사항
스크린샷
멘토에게