-
Notifications
You must be signed in to change notification settings - Fork 39
[맹은빈] sprint5 #189
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 #189
The head ref may contain hidden characters: "React-\uB9F9\uC740\uBE48-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주차 미션 제출 고생하셨습니다.
files 탭에서 보니 기존 코드들도 옮겨보시려고 하시는 것 같아요.
양은 많지만 학습에 분명 도움이 되실테니 추후 이어서 구현해보시는 것을 추천드려요!
다음 미션도 화이팅입니다.
| @@ -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> |
| export default function App() { | ||
| return ( | ||
| <Routes> | ||
| <Route path='/' element={<IndexPage />} /> |
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={<IndexPage />} /> | |
| <Route index element={<IndexPage />} /> |
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.
❗️ 수정요청
빈 파일이네요~ 구조잡기 위해 넣으신 것 같은데 쓰실 때 추가하시는 것을 추천드려요!
그리고 일반적으로 컴포넌트인 경우 대문자로 시작하지만 다른 파일들은 instance와 같이 명명하시면 됩니다~
| <p | ||
| className={`${ | ||
| isItems ? 'text-[#3692ff]' : 'text-[#4b5563]' | ||
| } cursor-pointer`} | ||
| onClick={() => navigate('/items')} | ||
| > | ||
| 중고마켓 | ||
| </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.
❗️ 수정요청
사용하고 계신 라우팅 라이브러리에서는 네비게이션에서 사용되는 wrapping된 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.
💊 제안
하나의 파일이 불필요하게 깁니다! 그럴만한 이유가 있다면 괜찮으나 지금의 경우는 분리가 잘 되지 않아서 그런 것 같아 보여요.
우선 컴포넌트들을 분리하시고-pagenation, 베스트 상품 등- 로직들도 custom hook, util 등으로 분리하시는 것을 추천드립니다!
| const topFavoriteProducts = [...favoriteProduct] | ||
| .sort((a, b) => b.favoriteCount - a.favoriteCount) | ||
| .slice(0, isMobile ? 1 : isTablet ? 2 : 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.
❗️ 수정요청
API 호출시 좋아요순으로 sort해서 가지고 오셔서 프론트에서 한번더 sort하실 필요가 없습니다~
slice 메소드는 원복 배열은 변경시키지 않으니 아래처럼 작서하시면 됩니다.
| const topFavoriteProducts = [...favoriteProduct] | |
| .sort((a, b) => b.favoriteCount - a.favoriteCount) | |
| .slice(0, isMobile ? 1 : isTablet ? 2 : 4); | |
| const topFavoriteProducts = favoriteProduct.slice(0, isMobile ? 1 : isTablet ? 2 : 4); |
| <input | ||
| type='text' | ||
| placeholder='검색할 상품을 입력해주세요' | ||
| className='border px-3 py-2 rounded flex-grow md:flex-grow-0 h-[4.2rem] text-2xl' | ||
| /> |
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.
💊 제안
요구사항에는 없었지만 상품 검색기능도 구현하시면 더 좋을 것 같아요!
| <button | ||
| onClick={() => { | ||
| navigate('/add'); | ||
| }} | ||
| className='bg-blue-500 text-white px-[2.3rem] py-[1.2rem] whitespace-nowrap text-2xl rounded-3xl cursor-pointer' | ||
| > | ||
| 상품 등록하기 | ||
| </button> |
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.
💊 제안
이런 버튼의 경우 link 가 더 적절할 것 같습니다.
| {startPage > 1 && ( | ||
| <button | ||
| onClick={() => setCurrentPage(startPage - 1)} | ||
| className='px-3 py-1 rounded border bg-gray-200' | ||
| > | ||
| < | ||
| </button> | ||
| )} |
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.
💊 제안
특정 조건에서 prevButton이 보이지 않게 처리하신 점 좋습니다.
다만 이렇게 되면 레이아웃이 흔들리게 되므로 button disabled 속성으로 처리하시는 것을 추천드립니다!
요구사항
배포 링크
https://serene-torte-71ea00.netlify.app/
기본
중고마켓 반응형
베스트 상품
전체 상품
심화
주요 변경사항
스크린샷
멘토에게