[유원규] sprint5 #223
[유원규] sprint5 #223
Conversation
[Fix] delete merged branch github action
…sion into React-유원규-sprint5
- 상품보기의 css를 수정하여야 합니다.
… 수정, SVG 아이콘 파일 추가
- api get으로 상품들 가져옴
GANGYIKIM
left a comment
GANGYIKIM
left a comment
There was a problem hiding this comment.
원규님 5번째 미션 작업 수고하셨습니다.
코멘트 반영은 필수가 아닌 선택이므로 참고해서 확인해주세요.
또한 중복된 내용의 경우 한번만 코멘트를 남기니, 해당 코멘트 반영시 비슷한 다른 내용도 찾아서 수정해보시면 더 좋을 것 같습니다!
다음 미션도 화이팅입니다~
| <BrowserRouter> | ||
| <Nav /> | ||
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="/freeboard" element={<FreeBoardPage />} /> | ||
| <Route path="/items" element={<ItemsPage />} /> | ||
| <Route path="/additem" element={<AddItem />} /> | ||
| </Routes> | ||
| </BrowserRouter> |
There was a problem hiding this comment.
💊 제안
header가 특정 페이지들에서 동일하게 들어가므로 이런 경우 App에서 추가하는 것보다 route에서 처리하시는 것이 관리 및 성능 측면에서 좋습니다.
홈, 로그인/회원가입, 그외 이렇게 3가지 경우에 따라 header가 다르게 보이므로 문서를 참고하셔서 수정해보세요~
https://reactrouter.com/start/declarative/routing#layout-routes
| <BrowserRouter> | ||
| <Nav /> | ||
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> |
There was a problem hiding this comment.
💊 제안
index 속성을 이용해 시작페이지라는 것을 더 명확하게 알 수 있게 하시는 것을 추천드립니다~
| <Route path="/" element={<HomePage />} /> | |
| <Route index element={<HomePage />} /> |
There was a problem hiding this comment.
💊 제안
지금 코드만으로 보면 버튼 태그보다 Link 태그를 쓰는게 더 적절해보여요.
단순 페이지 이동이 아니라 특정 로직을 실행해야한다면 지금처럼 버튼 태그를 사용하셔도 좋지만 아니라면 Link 태그로 변경해주세요~
링크 태그를 사용하시면 마우스 오른쪽 클릭이나 다른 기능도 제공이 가능해서 UX 측면에서도 좋습니다.
| <NavLink to="/" className="flex items-center gap-8"> | ||
| <img | ||
| src={Favicon} | ||
| alt="로고그림" | ||
| className="size-40 hidden tablet:block" | ||
| /> | ||
| <img src={logoText} alt="로고글씨" className="w-81 h-40" /> | ||
| </NavLink> |
There was a problem hiding this comment.
💊 제안
이 친구는 해당 위치에 있다고 해서 특정 기능을 요구하는 것이 아니라서 Link 태그가 더 적절할 것 같아요!
| <NavLink | ||
| to="/freeboard" | ||
| className={({ isActive }) => | ||
| isActive ? "text-primary-100" : "" | ||
| } | ||
| > | ||
| 자유게시판 | ||
| </NavLink> |
|
|
||
| const Dropdown = ({ onChange }) => { | ||
| const [selected, setSelected] = useState("list"); | ||
| const [open, setOpen] = useState(false); |
There was a problem hiding this comment.
💊 제안
open 이라는 이름은 함수처럼 보이네요~ 값을 추론할 수 있는 isOpen같은 이름을 추천드립니다.
| const isTablet = window.matchMedia("(min-width: 768px)").matches; | ||
|
|
||
| const handleSelect = (value) => { | ||
| const apiValue = value === "list" ? "recent" : "favorite"; |
There was a problem hiding this comment.
❗️ 수정요청
드롭다운에서 "최신순"의 경우 "list" 가 아니라 "recent"로 하는게 더 적절할 것 같아요.
아마 검색어를 입력하는 것에 따라 구분하시기 위해서 이렇게 하신 것 같은데, 검색어가 적용되어도 최신순, 좋아요순으로 동작을 해야하니 다른 코멘트를 참고하셔서 수정해보세요!
| <div | ||
| className="gap-24 cursor-pointer flex align-center justify-center border-1 border-gray-200 rounded-xl py-12 w-130 h-42 text-lg font-regular" | ||
| onClick={() => setOpen((o) => !o)} | ||
| > |
There was a problem hiding this comment.
❗️ 수정요청
이런 클릭 가능한 요소의 경우 버튼을 사용해주세요. 더 적절한 태그가 있는데 div 태그에 onClick 을 주게되면 button에게 기대하는 동작을 제대로 수행하지 못합니다!
| <div | |
| className="gap-24 cursor-pointer flex align-center justify-center border-1 border-gray-200 rounded-xl py-12 w-130 h-42 text-lg font-regular" | |
| onClick={() => setOpen((o) => !o)} | |
| > | |
| <button | |
| type="button" | |
| className="gap-24 cursor-pointer flex align-center justify-center border-1 border-gray-200 rounded-xl py-12 w-130 h-42 text-lg font-regular" | |
| onClick={() => setOpen((o) => !o)} | |
| > |
밑의 드롭다운 메뉴도 동일합니다~ 버튼 태그로 수정하시는 것을 추천드려요!
There was a problem hiding this comment.
❗️ 수정요청
지금 배포된 사이트의 경우, 모바일 사이즈에서 70page로 이동후 다시 PC 사이즈로 변경하면 빈 아이템 리스트가 보여집니다.
이렇게 화면이 변경되는 경우 적절한 페이지로 이동될 수 있게 로직을 추가해주세요~
There was a problem hiding this comment.
❗️ 수정요청
404 에러를 피하기 위해 _redirects 파일을 추가해주신 점은 좋습니다. 다만 해당 파일은 배포 최상단에 위치해야합니다.
지금 배포 사이트에서는 refresh 시 404에러가 발생하므로 이를 참고해서 수정해보세요~
4 participants
요구사항
https://sprint50527.netlify.app
기본
중고마켓 반응형
베스트 상품
전체 상품
심화
주요 변경사항
스크린샷
멘토에게