-
Notifications
You must be signed in to change notification settings - Fork 39
[양예지] Sprint5 #179
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 #179
The head ref may contain hidden characters: "React-\uC591\uC608\uC9C0-sprint5"
Conversation
…ithub-actions [Fix] delete merged branch github action
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.
수고하셨습니다!
이전 스프린트 미션 파일들 (html, js 등)은 정리해주시거나 React로 바꿔주시면 좋을것같아요.
전체적으로 큰 실수없이 잘 해주셨는데 useEffect 사용할때 불필요한 dependency와 결합하는것만 주의해주시면 좋을것같네요 :)
주요 리뷰 포인트
- useEffect 관련 리팩토링
- 훅 사용 제약조건 잘 이해하기
- Pagination 관련 로직, UI 재사용성 높이기
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.
기존 html파일을 유지하지마시고 리액트 기반으로 바꿔주세요!
아래와 같은 구조로 리턴문이 깔끔하게 유지되면 좋을것같습니다.
return (
<Header />
<HeroBanner />
<Feature props={...} />
<Feature props={...} />
<Feature props={...} />
<Footer />
)| if (width >= 1280) { | ||
| bestCount = 4; | ||
| } else if (width >= 768) { | ||
| bestCount = 2; | ||
| } else { | ||
| bestCount = 1; | ||
| } | ||
|
|
||
| let itemsPerPage; | ||
| if (width >= 1280) { | ||
| itemsPerPage = 10; | ||
| } else if (width >= 768) { | ||
| itemsPerPage = 6; | ||
| } else { | ||
| itemsPerPage = 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.
요거 혹시 워닝 안났나요? 훅 사용 시 if문같은 조건문 다음에는 사용할수 없고 최상위 컨텍스트에서만 쓰여야한다는 제약 조건이 있는데, 아마 eslint를 사용하지않아 syntax 검증에 대한 워닝을 못보신것같아요 ㅎㅎ 리액트부터는 eslint & prettier 모두 사용해주세요!
| useEffect(() => { | ||
| axios.get("https://panda-market-api.vercel.app/products").then((res) => { | ||
| const list = res.data.list; | ||
| console.log(res.data.list); | ||
| // 좋아요순 정렬 → 베스트 상품용 | ||
| const best = [...list] | ||
| .sort((a, b) => b.favoriteCount - a.favoriteCount) | ||
| .slice(0, 4); // 데스크탑 기준, JS로 개수 조절할 수도 있음 | ||
| setBestItems(best); | ||
|
|
||
| // 정렬 기준 따라 전체 상품용 | ||
| const sorted = sortItems(list, sortType); | ||
| setItems(sorted); | ||
| }); | ||
| }, [sortType]); |
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.
useEffect를 베스트 상품용 / 전체 상품용 두개로 나눠서 써보는건 어떨까요?
베스트 상품용 state update와 무관하게 sortType dependency는 전체 상품용에만 필요한데, 불필요한 결합이 생기고 있습니다.
동시성 모드부터 useEffect 훅은 최소 두번, 필요할때마다 계속 호출되어 사용되기때문에 이런 의존성 문제는 특히 조심해주시는게 좋습니다 :)
| ); | ||
| } | ||
|
|
||
| function Pagination({ total, perPage, current, setCurrent }) { |
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 컴포넌트는 페이지네이션이 필요할때마다 재사용되어야할텐데 공용 컴포넌트로 분리해주는건 어떨까요? :)
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.
그리고 로직 또한 재사용될텐데 (페이지 넘버 계산 등의 반복되는 작업들) 이런건 프로그램 내에서 여기저기 흩어져있기보단 커스텀훅으로 분리해주면 좋겠죠?
스프린트 미션 5
체크리스트 [기본]
중고마켓
중고마켓 반응형
체크리스트 [심화]