-
Notifications
You must be signed in to change notification settings - Fork 39
[김서연] Sprint5 #180
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 #180
The head ref may contain hidden characters: "React-\uAE40\uC11C\uC5F0-sprint5"
Conversation
- 주요 페이지 라우팅 설정 (Home, Login, Signup ... ) - 폴더 구조 정리 (componenets, pages, style) - 컴포넌트 폴더 생성 (Auth, Home-(Banner, Header, ContentSection ...)) - 이미지 파일 불러오기
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번째 미션 제출 고생하셨습니다!
기존의 작업하신 페이지들도 옮기시느라 시간이 많이 걸리셨을 것 같은데 대단합니다!
이는 필수 사항은 아니니 시간 되실때 마무리하시면 좋겠습니다.
-
메인페이지에서 모바일 사이즈 반응형이 적용되지 않고 있는데, 원인을 모르겠습니다.: 제가 봤을 때는 css에서 모바일 디자인 관련 코드가 없어보여요. 예를들어 content-card의 경우도 배포해주신 사이트에서 확인했을 때는 모바일 화면에서도 width가 67.6rem을 가지게 작업되어 있네요! 다만 이는 지금 요구사항의 필수 구현 사항은 아니니 추후 시간되실때 수정해보세요. -
header를 sticky로 고정해두었는데, 모바일에서는 적용되지 않는 이유를 모르겠습니다.: 이 문제는 mdn의 문서를 읽어보시면 이해하시는데 도움이 되실거에요. 우선적으로 이유를 말씀드리자면 모바일일때 body 에 주신 overflow-x 때문입니다~
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.
💊 제안
한 레포안에서는 동일한 룰을 따르는 것이 좋습니다.
따라서 공백 구분시 _와 - 중 하나만 사용하셔서 같은 네이밍 룰을 따르시는 것을 추천드려요~
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.
💬 여담
React Router를 사용하고 계신데 향후 라우트가 많아지면 Route들을 별도의 파일로 분리하면 App 컴포넌트가 더 깔끔해지고, 라우트 관리도 쉬워집니다.
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.
💊 제안
Banner라는 컴포넌트 명보다 Header가 더 적절할 것 같아요. 일반적으로 Banner는 광고영역을 의미합니다.
| <NavLink to="/community" style={getLinkStyle}> | ||
| 자유게시판 | ||
| </NavLink> |
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.
👍 칭찬
NavLink 쓰신 거 좋아요!
| <Link className="item-logo" to="/"> | ||
| <img src={logo} alt="판다마켓 로고 이미지" /> | ||
| </Link> | ||
| <Link className="item-logo-text" to="/"> | ||
| <img src={logo_title} alt="판다마켓" /> | ||
| </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.
💊 제안
아래가 더 적절한 구분일 것 같아요~
| <Link className="item-logo" to="/"> | |
| <img src={logo} alt="판다마켓 로고 이미지" /> | |
| </Link> | |
| <Link className="item-logo-text" to="/"> | |
| <img src={logo_title} alt="판다마켓" /> | |
| </Link> | |
| <Link to="/"> | |
| <img className="item-logo" src={logo} alt="판다마켓 로고 이미지" /> | |
| <img className="item-logo-text" src={logo_title} alt="판다마켓" /> | |
| </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.
❗️ 수정요청
상품 검색 기능이 구현되지 않은 것 같아요! 추가 구현해주시면 더 좋겠네요~
| /> | ||
| <div className="item-card-description"> | ||
| <span className="item-name">{item.name}</span> | ||
| <h2 className="item-price">{item.price}</h2> |
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.
💊 제안
금액의 경우 (,)를 통해 단위가 구분되도록 아래처럼 작성해주세요!
| <h2 className="item-price">{item.price}</h2> | |
| <h2 className="item-price">{item.price.toLocaleString()}</h2> |
| function ItemList({ items, className }) { | ||
| return ( | ||
| <div className={className}> | ||
| {items?.map((item) => ( | ||
| <ItemCard key={item.id} item={item} className={className} /> | ||
| ))} | ||
| </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.
💊 제안
ItemList에서 className props를 받아 이를 div에도 넘기고 ItemCard에도 넘기는 형태는 좀 헷갈리는 것 같아요!
만약 이렇게 받으셔야 한다면 prop명을 더 자세하게 적으시고 wrapperClassName, className 이렇게 따로 받으시는 것을 추천드려요.
| <li | ||
| key={option.value} | ||
| className="dropdown-item" | ||
| onClick={() => handleOptionClick(option.value)} | ||
| > | ||
| {option.label} | ||
| </li> |
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.
❗️ 수정요청
이런 클릭 가능한 요소의 경우 버튼을 사용해주세요. 더 적절한 태그가 있는데 이미지 태그에 onClick 을 주게되면 button에게 기대하는 동작을 제대로 수행하지 못합니다!
| <li | |
| key={option.value} | |
| className="dropdown-item" | |
| onClick={() => handleOptionClick(option.value)} | |
| > | |
| {option.label} | |
| </li> | |
| <li | |
| key={option.value} | |
| className="dropdown-item" | |
| > | |
| <button type="button" value={option.value} onClick={handleOptionClick}>{option.label}</button> | |
| </li> |
위에처럼 button 태그로 바꾸시고 onClick 시 event 객체에서 button 태그의 value를 받아오는 방식으로 구현하시는 것을 추천드려요!
| useEffect(() => { | ||
| window.addEventListener("resize", debouncedResize); | ||
|
|
||
| return () => { | ||
| window.removeEventListener("resize", debouncedResize); | ||
| debouncedResize.cancel(); | ||
| }; | ||
| }, [debouncedResize]); |
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.
💊 제안
React 공식문서의 useState 주의사항을 보시면 setState를 통해 넣어준 값이 기존 값과 동일한 경우, 렌더링을 유발하지 않습니다.
따라서 리렌더링을 막기위해 debounce를 쓰실 필요가 없어보입니다~
https://ko.react.dev/reference/react/useState#setstate-caveats
요구사항
배포 사이트
https://sprint-mission5-pandamarket.netlify.app/
기본
중고마켓
중고마켓 반응형
베스트 상품
전체 상품
심화
스크린샷
PC

태블릿


모바일


멘토에게