-
Notifications
You must be signed in to change notification settings - Fork 39
[유용민] sprint5 #186
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 #186
The head ref may contain hidden characters: "React-\uC720\uC6A9\uBBFC-sprint5"
Conversation
[Fix] delete merged branch github action
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번째 미션 작업 고생하셨습니다!
이번 React 미션처럼 다음 Next 미션도 화이팅입니다!
-
지금 작업하신 상품 페이지의 경우, 전체 상품을 화면 사이즈에 따라 css로 보이고 안 보이게 처리하고 있습니다. 이렇게 되면! 화면이 작을때는 특정 아이템들이 영원히 보이지 않게 됩니다! css 로 처리하시는 것은 베스트 아이템만 하시고 전체 상품의 경우는 쿼리로 불러오시는 개수를 조절하시는 것이 좋겠습니다.
-
베스트 상품이랑 전체 아이템이랑 보여주는 방식이 같길래 ProductList라는 컴포넌트 하나로 처리하려고 했다가 반응형에서 엄청 고생해서 그냥 2개로 나눴습니다.. 그런데 지금 보면 또 될 것 같기도 하네요..:
컴포넌트 단위가 클필요가 없어서 저는 나누신 것이 더 좋다고 생각합니다. -
페이지네이션이 생각보다 어려웠습니다..:
제 생각에 pages/Products에 코드가 몰려있어 파악하기 힘드셔서 더 어려우셨을 것 같아요.
추후 컴포넌트를 작성하실때 분리를 잘 하시고 필요한 기능을 미리 적으신 다음 작업하시면 더 수월하실거에요. -
전체 상품 쪽 검색/등록/ 드랍다운을 반응형으로 만들기 위해 고민했지만, 조건부 렌더링으로 결론을 내렸는데 다른 방법이 있었을까요?:
우선 이번 디자인의 경우 제 생각에는 css로 처리하는 것이 가능한 디자인이라 미디어 쿼리를 이용해 작업하시는 것이 가장 좋았을 것이라고 생각합니다. 다만 불가피하게 화면 사이즈별로 달라야하는경우, 겹치는 영역이 있다면 해당 영역은 재사용하고 필요한 부분만 조건부 렌더링을 하고 조건부 렌더링 되는 컴포넌트 내부에서도 동일한 디자인이 있다면 컴포넌트로 선언해 재사용하는 것이 좋다고 생각합니다.
// case1 조건부 렌더링 되는 영역이 큽니다.
{isMobile && <MobileHeader />}
{!isMobile && <Header/>}
// case2 조건부 렌더링 되는 영역이 작고 어떤 부분이 화면별로 다른지 파악하기 쉽습니다.
<Header>
<SearchBar/>
<Button>Create</Button>
{!isMobile && <Dropdown/>}
{isMobile && <IconDropdown/>}
</Header>
| @@ -0,0 +1,13 @@ | |||
| <!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"> |
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="/vite.svg" /> | ||
| <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> |
| import { | ||
| API_BASE_URL, | ||
| API_HEADERS, | ||
| API_TIMEOUT, | ||
| } from "../constants/apiConstants"; |
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.
💊 제안
상수로 빼주신 것은 좋지만 해당 값들이 재활용되는 값인지, 분리되어 있는 것이 더 좋을지 잘 모르겠습니다~
colocation이라고 관련된 값들이 같은 파일에 있는 것이 좋을 떄가 있습니다.
이런 API_BASE_URL, API_HEADERS, API_TIMEOUT 값들이 얼마나 반복사용되는지는 잘 모르겠습니다만
한번 고민해보시면 좋겠습니다~
| }; | ||
|
|
||
| // 응답 인터셉터 - 서버에서 내려주는 message를 그대로 사용 | ||
| instance.interceptors.response.use( |
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.
👍 칭찬
interceptors로 공통 에러 처리 좋아요!
| import noImage from "../assets/no-image.png"; | ||
|
|
||
| export default function AllProductList({ data = [] }) { | ||
| if (!data.length) return null; |
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.
💊 제안
data가 없을 경우 이를 알 수 있는 컴포넌트를 보여주시면 좋습니다~
상품이 없을 경우 "상품이 없습니다", 검색시 결과가 없을 경우 "검색 결과가 없습니다" 와 같은 문구들이 이에 해당됩니다.
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.
💊 제안
tailwind에서 base 스타일을 추가하실때는 tailwind base 레이어에 추가하시는 것을 추천드려요!
https://tailwindcss.com/docs/adding-custom-styles#adding-base-styles
| const filteredData = data.map((item, index) => { | ||
| let hiddenClass = ""; | ||
|
|
||
| if (index >= display.mobile) { | ||
| hiddenClass += " max-md:hidden"; | ||
| } | ||
| if (index >= display.tablet) { | ||
| hiddenClass += " md:max-xl:hidden"; | ||
| } | ||
| if (index >= display.desktop) { | ||
| hiddenClass += " xl:hidden"; | ||
| } | ||
|
|
||
| return { ...item, hiddenClass }; | ||
| }); |
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.
❗️ 수정요청
data 값을 이용하지 않는데 불필요하게 순회하면서 처리할 필요 없을 것 같아요.
jsx에서 순회할때 index를 가지고 classname을 반환하는 함수로 빼시는 것을 추천드려요!
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.
❗️ 수정요청
AllProduct와 BestProduct 내부에서 디자인적으로도 로직상에서도 겹치는 부분이 보이네요.
로직은 custom hook이나 util로 빼시고 반복되는 디자인은 컴포넌트로 빼시면 좋겠습니다.
| const getBestItems = async () => { | ||
| try { | ||
| const data = await getItems({ | ||
| orderBy: "favorite", | ||
| page: 1, | ||
| pageSize: 4, | ||
| }); | ||
| console.log("best", data); | ||
| setBestItems(data.list); | ||
| } catch (error) { | ||
| setError(error.message); | ||
| } | ||
| }; |
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.
❗️ 수정요청
bestItems의 경우 AllItems와 다르게 재호출될 필요가 없어서 BestProducts 컴포넌트 내부에서 호출하셔도 될 것 같아요. 만약 UI와 비즈니스 로직을 분리하기 위해서 이렇게 작성하셨다면 해당 파일 내부에서 로직들을 관심사별로 분리하시면 좋을 것 같아요.
지금의 경우 해당 파일이 너무 깁니다~
우선 페이지네이션은 꼭 분리하시면 좋겠어요!
| <input | ||
| className="w-full py-3 px-14 rounded-2xl text-[1.4rem] bg-[#F3F4F6] text-[#9CA3AF]" | ||
| 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.
💊 제안
요구사항에는 없었지만 상품 검색기능도 구현하시면 더 좋을 것 같아요!
요구사항
-배포주소: https://15-sprint-mission-tawny.vercel.app/
기본
중고마켓
중고마켓 반응형
심화
주요 변경사항
스크린샷
멘토에게