-
Notifications
You must be signed in to change notification settings - Fork 21
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
[김가희] sprint6 #92
The head ref may contain hidden characters: "React-\uAE40\uAC00\uD76C-week6"
[김가희] sprint6 #92
Conversation
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.
가희님 이번 스프린트 미션하시느라 고생하셨습니다!
아직 작업중이라고 하셨으니 제 피드백은 참고만해주시고
가희님이 생각하시는 방향으로 작업해보세요 👍
- 체크리스트에서 작업하신 사항에 대해 체크를 해주시면 리뷰드릴때 참고합니다.
- 중복되는 사항에 대해서는 한번만 리뷰를 답니다. 원하실 때 비슷한 사항들을 찾아서 같이 바꿔보세요~
- 컴포넌트나 로직을 분리하신것 좋습니다.
.dropBox { | ||
} |
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.
P3:
빈 CSS block은 없는 것이 가독성에 더 좋습니다~
<form className={style.dropBox}> | ||
<select onChange={handleChange}> | ||
<option value="recent">최신순</option> | ||
<option value="favorite">좋아요순</option> | ||
</select> | ||
</form> |
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.
P1:
selectbox 태그를 사용시 css 로 구현하는데 한계가 있습니다.
디자인대로 구현을 하기 위해서는 스스로 selectbox를 만드셔야합니다.
만약 이렇게 구현하는 것이 어렵다면, 아래 글을 참고해서 selectbox의 외관이라도
디자인과 동일하게 변경해주시면 더 좋을 것 같습니다.
@font-face { | ||
font-family: "Pretendard Variable"; | ||
src: url("../../public/Pretendard-Regular.ttf"); | ||
} |
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.
P1:
font의 경우 App이나 index같은 상위 컴포넌트의 html, body태그에 지정해주면
해당 태그의 자식 태그들은 해당 속성을 상속받으므로 이와 같이 반복해서 font-face, font-famliy를 지정해줄 필요가 없게 됩니다.
가능하면 상위에서 해당 속성을 주시길 추천드리겠습니다.
<div className={styles.navigateUi}> | ||
<div>자유게시판</div> | ||
<div>중고마켓</div> | ||
</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.
P2:
가희님께서는 react-router-dom을 사용하게 계시므로
이렇게 페이지 이동을 해야하는 경우 a tag보다, 해당 라이브러리의 태그를 사용하시길 귄장드립니다.
*react-router-dom 문서
https://reactrouter.com/en/main/components/nav-link
{cardList.map((item) => { | ||
return <Card key={item.id} item={item} imgSize={"282px"}></Card>; | ||
})} |
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.
P3:
아래코드도 동일 동작합니다~
{cardList.map((item) => { | |
return <Card key={item.id} item={item} imgSize={"282px"}></Card>; | |
})} | |
{cardList.map((item) => ( | |
<Card key={item.id} item={item} imgSize={"282px"}></Card> | |
))} |
// // 1페이지 10개 최신순 줘 | ||
// const 1페이지10개 = getItems(1,10,"recent") | ||
// const 2페이지10개 = getItems(2,10,"recent") | ||
// const 3페이지10개 = getItems(3,10,"recent") | ||
|
||
// // 최신순으로 1페이지 1개 줘 | ||
// const 1페이지10개 = getItems(1,10,"favorite") | ||
// const 2페이지10개 = getItems(2,10,"favorite") | ||
// const 3페이지10개 = getItems(3,10,"favorite") |
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.
P3:
테스트를 해보시느라 주석을 남겨두신 것 같은데
이런 주석은 없는 것이 가독성에 더 좋습니다~
코드 베이스에 올라갈 이유가 없는 코드들은 commit 하실때 지우고 올리시는 걸 추천드려요
export async function getItems( | ||
page = 1, | ||
pageSize = 10, | ||
orderBy = "recent", | ||
keyword | ||
) { |
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.
P2:
모든 파람의 경우 default값이 설정되어 있는데, 이런 방식으로는 default값을 설정해두신 장점을 살리기 어렵습니다.
아래처럼 obj로 받으시면 필요한 값만 넘길 수 있으므로 아래와 같은 방식도 고려해보세요~
export async function getItems({
page = 1,
pageSize = 10,
orderBy = "recent",
keyword
}) { ... }
// BestList.js에서 사용시
const fetchData = async function () {
const data = await getItems({ pageSize: 4, orderBy: "favorite" });
setCardList(data.list);
};
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.
P3:
itemList와 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.
P2:
페이지 10에서 ">>" 버튼을 눌렀을 때 이동이 되지 않네요~
아마 서버에 저장된 현재 아이템 개수가 139개이고 14 페이지까지 존재하는 상황에서
currentPage10 + pageToShow5를 더하려고 하니 15 페이지라서 이렇게 동작하는 것 같습니다.
만약 이것이 의도하신 동작이라면 nextPage, prevPage 버튼에 위와 같은 경우 disabled를 해주시는 것을 추천드리고
의도하지 않은 동작이시라면 위와 같은 경우 현재 페이지중 마지막페이지인 14 페이지로 이동시키시는 걸 추천드립니다.
요구사항
체크리스트 [기본]
상품 등록
체크리스트 [심화]
상품 등록
주요 변경사항
스크린샷
멘토에게