-
Notifications
You must be signed in to change notification settings - Fork 39
[김진선] sprint5 #164
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 #164
The head ref may contain hidden characters: "React-\uAE40\uC9C4\uC120-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.
수고하셨습니다!
전체적으로 코드가 읽기 편안하고 깔끔하네요 ㅎㅎ
몇가지 리팩토링하실때 도움될만한 코멘트 남겨드렸으니 참고해보세요! :)
주요 리뷰 포인트
- 디렉토리 구조
- 리액트 훅 올바로 사용해보기
- 메모리 사용 효율적으로하기
- z-index 이슈 사전 방지
- useState 초기값 동기적 함수 실행에 대한 피드백
| </header> | ||
| <div className="app"> | ||
| <Routes> | ||
| <Route element={<DefaultLayout />}> |
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.
아무래도 라우트에 쓰는 컴포넌트 이름이다보니, 일관성을 위해 라우트에 쓰이는 다른 페이지 단위의 컴포넌트와 비슷하게 Layout이라는 단어를 빼고 Root정도로 네이밍을 바꾸는게 좋지않을까싶네요 :)
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.
전체적인 레이아웃으로 사용하기 위해서 layouts폴더 안에 따로 만들어서 사용중인 컴포넌트라서 DefaultLayout이라고 네이밍을 했었는데 Root가 더 적당한 네이밍일까요?
저 같은 경우는 역할을 명확하게 알 수 있어서 DefaultLayout이라고 네이밍을 했었거든요.
네이밍 관련해서는 항상 고민이 많이 되는 것 같습니다.🤔
| @@ -0,0 +1,2 @@ | |||
| export 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.
보통 이런 민감한 정보의 경우 git 노출을 막기위해 환경변수로 관리해준답니다! :)
리액트에서 환경 변수를 사용하려면 변수명에 REACT_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.
프로젝트 최상위 위치에 .env파일을 생성해서 환경변수를 만들어줬습니다.
이름도 좀 더 명확히 하기 위해서 BASE_URL→BASE_API_URL로 바꾸어 주었습니다.
| const [page, changePage] = usePaginationState(limit); | ||
| const [sort, setSort] = useState("latest"); | ||
|
|
||
| const { products, totalPages } = usePaginatedProducts({ page, limit, sort }); |
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.
리액트 훅은 내부적으로 호출 순서에 의존합니다. 참고
따라서 이 로직은 약간의 문제 소지가 발생할수있는 구조인데,
usePaginatedProducts는 usePaginationState 훅 사용에 의해 업데이트되어야하는 page에 의존하고있기때문에, 호출 순서 실수가 일어나게되면 로직이 깨져버립니다.
커스텀 훅을 하나만 만드시고 해당 훅에서 상태 업데이트 호출순서를 제어하시는게 더 안정적인 설계 방법이 될것같네요 :)
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.
useProductsPagination이라는 커스텀 훅을 하나만 만들어서 업데이트 호출순서를 제어하도록 만들어봤습니다.
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 폴더는 여러 페이지에서 사용하는 공용 컴포넌트를 모아놓는 용도로 활용되는게 일반적이기때문에, 공용 컴포넌트가 아니라면 사용하는 위치에서 가깝게 파일 위치를 바꿔주시는게 좋을것같아요!
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.
말씀대로 여러곳에서 재사용되는 게 아니라 Items페이지에서만 재사용되는 컴포넌트들을 공용 컴포넌트에 넣는건 프로젝트가 복잡해지면 관리하기 힘들 것 같아서 pages/Items/폴더 안에 components폴더를 만들어서 폴더 구조를 정리해봤습니다.
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.
pages/Items/폴더 안에 hooks폴더도 만들어서 폴더 구조를 정리해봤습니다!
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.
처음에 옮겼다가 단일 컴포넌트에서만 사용이 된다고 판단돼서 삭제하고 BestProducts컴포넌트에 합쳤습니다.
|
|
||
| function BestProducts({ title, itemsPerDevice}) { | ||
|
|
||
| const bestProducts = useBestProducts(itemsPerDevice); |
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.
로직 재사용 용도가 아니라면 이렇게 한번 더 래핑해서 useBestProducts 훅을 따로 만들어줄 필요가 있을까요?
오히려 컴포넌트단에서 고유의 데이터인 state를 사용해 직접적으로 데이터 fetching 로직을 처리하는 방식이 좀더 직관적일것같아요.
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.
말씀대로 useBestProducts훅 같은 경우에는단일 컴포넌트 전용이며 단순하기 때문에 직관적으로 BestProducts 내부에 로직을 통합하였습니다.
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.
이미지에 관련한 처리를 공통적으로 재사용하기위해 따로 컴포넌트를 만드셨군요! 굳굳 👍 좋은 시도입니다 :)
| function Pagination({ | ||
| currentPage, | ||
| totalPages, | ||
| onPageChange, | ||
| maxPageButtons = 5, | ||
| }) { |
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.
매직 넘버를 사용하지않고, 상수로 분리해보면 어떨까요?
| function Pagination({ | |
| currentPage, | |
| totalPages, | |
| onPageChange, | |
| maxPageButtons = 5, | |
| }) { | |
| const GROUP_SIZE = 5; | |
| function Pagination({ | |
| currentPage, | |
| totalPages, | |
| onPageChange, | |
| maxPageButtons = GROUP_SIZE, | |
| }) { |
이렇게 매 렌더링때마다 재계산 될 필요없는 상수의 경우, 컴포넌트 외부로 빼두면 상수가 한 번만 생성되고 재사용되므로 메모리 사용을 더 효율적으로 할 수 있답니다 :)
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 컴포넌트 외부에서 재사용될 일이 거의 없을 거라고 생각되는 상수라서 constants.js로 빼지는 않고 가르쳐주신대로 컴포넌트 파일 안에 정의하였습니다.
| {open && ( | ||
| <ul className={styles.dropdown}> | ||
| {options.map((opt) => ( | ||
| <li | ||
| key={opt.value} | ||
| className={`${styles.option} ${opt.value === value ? styles.selected : ''}`} | ||
| onClick={() => { | ||
| onChange(opt.value); | ||
| setOpen(false); | ||
| }} | ||
| > | ||
| {opt.label} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| )} |
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.
지금은 버튼 클릭 시 드롭다운을 띄우는 정도라서 이렇게 작성하신것같은데,
다른 컴포넌트와의 상호작용을 처리하다보면 쉽게 z-index 이슈가 생길 수 있어요.
바로 이런 문제때문에 z-index의 위계를 정해놓고 관리하시는게 좋은데요!
지금은 css modules를 쓰고계시니까
:root {
/* Base layers */
--z-base: 0;
--z-above-base: 1;
/* UI Components */
--z-dropdown: 1000;
--z-modal: 2000;
--z-toast: 3000;
--z-tooltip: 4000;
/* Fixed elements */
--z-header: 100;
--z-footer: 100;
--z-sidebar: 200;
/* Overlay elements */
--z-overlay: 1500;
--z-overlay-above: 2500;
} 이렇게 z-index 위계를 변수화해두고 사용하는걸 추천드립니다!
그리고 나중에 모달과 같이 최상위에 렌더링되어야하는 요소가 있다면, Portal을 사용해서 DOM 계층 구조의 최상위에 렌더링되게끔해 z-index나 overflow 문제를 쉽게 해결하실수도 있으니 참고해보세요 :)
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.
variables.css파일에 z-index 위계를 변수화해두고 드롭다운 쪽에 z-index설정을 해주었습니다.
추후에 Portal도 사용해 보도록 노력하겠습니다.
|
|
||
| export default function useResponsiveLimit(itemsPerDevice) { | ||
| const [limit, setLimit] = useState(() => | ||
| getLimitFromWindowWidth( |
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.
이 코드도 몇가지의 기술적 배경을 고려한다면 개선하시는게 좋습니다.
다른분 PR에 달아드린 코멘트인데, 참고해보시고 리팩토링해보세요!
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.
알려주신 예시를 참고해서 초기 limit값을 정해주고, resize이벤트 발생 시에 getLimitFromWindowWidth함수가 실행되도록 수정을 해보았습니다.
현재 환경에서는 100% CSR 환경이라 필요없겠지만 SSR환경을 대응해 본다면 typeof window === "undefined"조건을 사용해볼 수 있지 않을까 판단되어서 코드를 작성해 보았습니다.
이렇게 수정하면 UX 관점에서 약간 깜빡임이 생길 수 있다고 하는데 미션7에서 스켈레톤을 만들기 때문에 스켈레톤을 적용하면 괜찮을 거라고 판단했습니다.
질문에 대한 답변
네! 좋습니다. 차차 피드백을 토대로 개선해보시면 성장에 도움이 되실거예요 :) |
451995e to
af36e1b
Compare
요구사항
기본
중고마켓 페이지 주소는 “/items” 입니다.
페이지 주소가 “/items” 일때 상단네비게이션바의 '중고마켓' 버튼의 색상은 “3692FF”입니다.
상단 네비게이션 바는 이전 미션에서 구현한 랜딩 페이지와 동일한 스타일로 만들어 주세요.
상품 데이터 정보는 https://panda-market-api.vercel.app/docs/#/ 에 명세된 GET 메소드 “/products” 를 사용해주세요.
'상품 등록하기' 버튼을 누르면 “/additem” 로 이동합니다. ( 빈 페이지 )
전체 상품에서 드롭 다운으로 “최신 순” 또는 “좋아요 순”을 선택해서 정렬을 할 수 있습니다.
중고마켓 반응형
베스트 상품
심화
주요 변경사항
배포
https://reliable-kangaroo-be4864.netlify.app/
스크린샷
네비게이션 바 디자인
네비게이션 바 디자인을 반응형으로 구현하였습니다.
모바일
태블릿
데스크톱
반응형 디자인(모바일)
반응형 디자인(태블릿)
반응형 디자인(데스크톱)
상품정렬 - 최신순
상품정렬 - 좋아요순
페이지네이션
다른 디바이스 화면으로 변경 시 보고있던 상품 위치 유지
이전/다음 버튼 불필요 시 비활성화
멘토에게