-
Notifications
You must be signed in to change notification settings - Fork 39
[유동환] sprint5 #188
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 #188
The head ref may contain hidden characters: "React-\uC720\uB3D9\uD658-sprint5"
Conversation
GANGYIKIM
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.
동환님 5번째 리액트 미션 수고하셨습니다!
리액트로 작업하시는 것이 쉽지 않으셨을거에요.
그래도 요구사항을 다 구현하시고, 심화도 시도해보셔서 좋네요!
다음 미션들도 화이팅입니다~
- 다음에는 배포 주소도 같이 올려주시면 좋을 것 같아요!
- sort 드롭다운도 디자인대로 구현해보시면 더 좋을 것 같아요.
- 전체 상품영역의 헤더가 모바일일 때 레이아웃이 변경되게 디자인 되어 있으니 추후 확인해보시고 수정하시면 좋겠습니다.
| @@ -0,0 +1,12 @@ | |||
| <!doctype html> | |||
| <html lang="en"> | |||
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 lang="en"> | |
| <html lang="ko"> |
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Vite + React</title> |
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.
💊 제안
유저가 알 수 있게 타이틀 태그를 수정해주시고 미션에서 추가하셨던 메타 태그도 추가하시면 더 좋을 것 같아요!
| <title>Vite + React</title> | |
| <title>판다마켓</title> |
| <Header /> | ||
|
|
||
| <Routes> | ||
| <Route path="/" element={<Main />} /> |
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.
💊 제안
index 속성을 이용해 시작페이지라는 것을 더 명확하게 알 수 있게 하시는 것을 추천드립니다~
| <Route path="/" element={<Main />} /> | |
| <Route index element={<Main />} /> |
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.
💊 제안
emotions 문서에서 스타일 선언문들을 같은 파일에 둘 것을 권하고 있습니다~
공식 문서를 읽어보시고 이러한 방향도 고민해보세요!
https://emotion.sh/docs/best-practices#colocate-styles-with-components
| @@ -0,0 +1,10 @@ | |||
| export async function getItems({ page=1, pageSize=100, orderBy="recent"}) { | |||
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의 default value가 너무 큰 것 같아요! 프론트에서 필요한 만큼만 불러오실거고 default value는 가장 많이 사용되는 값을 넣는 것이 좋으니 10정도를 추천드려요!
| {bestItems.map((item) => { | ||
| return <ItemList key={item.id} item={item}/> | ||
| })} |
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.
💊 제안
순회를 돌며 단순히 jsx를 반환할 뿐이니 아래처럼 하시는게 가독성 측면에서 더 좋을 것 같아요.
| {bestItems.map((item) => { | |
| return <ItemList key={item.id} item={item}/> | |
| })} | |
| {bestItems.map((item) => <ItemList key={item.id} item={item}/> )} |
| )} | ||
| </Link> | ||
| <div css={NavlinkStyle}> | ||
| <Link to={"/FreeBoard"} css={aTag}>자유게시판</Link> |
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.
❗️ 수정요청
사용하고 계신 라우팅 라이브러리에서는 네비게이션에서 사용되는 wrapping된 link 컴포넌트를 제공합니다~
이를 통해 해당 컴포넌트들에서 일반적인 요구사항을 더 쉽게 구현할 수 있고, 컴포넌트의 역할도 더 명확해지니
아래 문서를 읽고 수정해보시면 더 좋겠습니다.
|
|
||
| /** @jsxImportSource @emotion/react */ | ||
|
|
||
| const ItemList = ({ item }) => { |
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.
💊 제안
product 라는 객체로 props를 받기보다 images, name... 이렇게 받으시는게 더 좋아보여요.
지금 해당 컴포넌트에서 다른 props를 받는것이 아니라서 객체로 받으실 필요가 없어보입니다~
// 추천하는 방식
const ExComponent = ({ images, name }) => { ... }
// 아래의 경우 다른 props를 받기 때문에 객체로 묶어 받음
const ExComponent = ({obj, classname}) => { ... }| alt={item.name} > | ||
| </img> | ||
| <p css={itemTitle}>{item.name}</p> | ||
| <p css={itemPrice}>{`${item.price}원`}</p> |
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.
💊 제안
금액의 경우 (,)를 통해 단위가 구분되도록 아래처럼 작성해주세요!
| <p css={itemPrice}>{`${item.price}원`}</p> | |
| <p css={itemPrice}>{`${item.price.toLocaleString()}원`}</p> |
| const handleLoad = async (order = orderBy) => { | ||
| const [best, all]= await Promise.all([ | ||
| getItems({page:1, pageSize:4, orderBy: "favorite"}), | ||
| getItems({page:1, pageSize:10, orderBy: order}), | ||
| ]); | ||
| setBestItems(best.list); | ||
| setAllItems(all.list); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| handleLoad(orderBy) | ||
| }, [orderBy]) |
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.
❗️ 수정요청
베스트 아이템은 처음에 한번 불러온 후 다시 호출될 필요가 없습니다. 다만 지금 코드에서는 전체 상품을 재호출할때마다 다시 불러와지고 있으니 이를 수정하시면 좋겠습니다!
요구사항
기본
중고마켓 반응형
베스트 상품
전체 상품
심화
주요 변경사항
스크린샷
데스크탑
태블릿
모바일
멘토에게