-
Notifications
You must be signed in to change notification settings - Fork 39
[김영현] Sprint6 #195
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
[김영현] Sprint6 #195
The head ref may contain hidden characters: "React-\uAE40\uC601\uD604-sprint6"
Conversation
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.
영현님 6번째 미션 작업 고생하셨습니다~
시간이 넉넉하지 않으셨는데 전에 드린 코멘트도 반영하시다니 대단하네요. 변경 사항에 대해서 자세히 적어주시고 배포도 해주셔서 잘 확인할 수 있었어요~
남은 미션들도 화이팅입니다!
- 배포주소에서 확인해보니 input들의 색상값이 회색으로 디자인과 다른것 같아요~
- 판매가격의 경우 number만 받는 것이 더 적절할 것 같아요.
| .best-products__text-group { | ||
| display: flex; | ||
| margin-top: 8px; | ||
| /* margin-top: 8px; */ |
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.
❗️ 수정요청
| /* margin-top: 8px; */ |
| <div className="additem"> | ||
| <div className="additem-header"> | ||
| <h2>상품 등록하기</h2> | ||
| <button className="submit-button" disabled={!isFormValid}> | ||
| 등록 | ||
| </button> | ||
| </div> | ||
|
|
||
| <div className="additem-image-section"> | ||
| <ImageUpload /> | ||
| </div> | ||
|
|
||
| <div className="additem-form-section"> | ||
| <FormInput onFormValidChange={setIsFormValid} /> | ||
| </div> | ||
| </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.
💊 제안
input들이 모여있는 것이니 form으로 태그 작성하시면 좋겠습니다.
form의 동작상 이점을 위해 아래처럼 submit 버튼을 포함하는 영역을 form으로 변경해보세요.
| <div className="additem"> | |
| <div className="additem-header"> | |
| <h2>상품 등록하기</h2> | |
| <button className="submit-button" disabled={!isFormValid}> | |
| 등록 | |
| </button> | |
| </div> | |
| <div className="additem-image-section"> | |
| <ImageUpload /> | |
| </div> | |
| <div className="additem-form-section"> | |
| <FormInput onFormValidChange={setIsFormValid} /> | |
| </div> | |
| </div> | |
| <form className="additem"> // 이렇게하면 input에서 submit 이벤트 발생시 submit 이벤트가 실행됩니다. | |
| <div className="additem-header"> | |
| <h2>상품 등록하기</h2> | |
| <button type="submit" className="submit-button" disabled={!isFormValid} onSubmit={onSubmit}> | |
| 등록 | |
| </button> | |
| </div> | |
| <div className="additem-image-section"> | |
| <ImageUpload /> | |
| </div> | |
| <div className="additem-form-section"> | |
| <FormInput onFormValidChange={setIsFormValid} /> | |
| </div> | |
| </form> |
| const handleKeyDown = (e) => { | ||
| if (e.key === "Enter") { | ||
| onSearch(keyword.trim()); | ||
| } | ||
| }; | ||
|
|
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.
💊 제안
onChange를 통해 값이 변경될때마다 onSearch를 업데이트 하고 있으니 해당 부분은 필요 없을 것 같아요.
만약 enter시에만 검색을 하게 동작하고 싶으시다면 handleChange를 지우시고 uncontrolled input 방식으로 변경하시면 더 적절할 것 같아요.
| <button onClick={() => nav("/additem")} className="product-list__add-button"> | ||
| <span>상품 등록하기</span> | ||
| </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.
💊 제안
해당 UI가 반복 사용되니 컴포넌트로 분리하시면 더 적절할 것 같아요.
| const params = { | ||
| page: 1, | ||
| pageSize: 300, | ||
| orderBy, | ||
| }; | ||
| if (searchKeyword) { | ||
| params.keyword = searchKeyword; | ||
| } |
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.
💬 여담
아래 코드도 동일동작합니다~
| const params = { | |
| page: 1, | |
| pageSize: 300, | |
| orderBy, | |
| }; | |
| if (searchKeyword) { | |
| params.keyword = searchKeyword; | |
| } | |
| const params = { | |
| page: 1, | |
| pageSize: 300, | |
| orderBy, | |
| ...(searchKeyword && { keyword: searchKeyword }) | |
| }; |
| <NavLink to="/items" className={isMarketPage ? "market-active" : ""}> | ||
| 중고마켓 | ||
| </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 컴포넌트를 사용하시니 내부적으로 제공하는 방식으로 처리해보세요~
| <NavLink to="/items" className={isMarketPage ? "market-active" : ""}> | |
| 중고마켓 | |
| </NavLink> | |
| <NavLink to="/items" className={({ isActive }) => isActive ? "market-active" : ""}> | |
| 중고마켓 | |
| </NavLink> |
해당 컴포넌트의 자세한 기능은 문서를 참고해보세요~
| <NavLink to="/items"> | ||
| <img src={isMobile ? mobileLogo : logo} alt="logo" className="header__logo-img" /> | ||
| </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 컴포넌트의 wrapping된 요소로 문서에서는 아래처럼 말하고 있습니다.
Wraps Link with additional props for styling active and pending states.
로고의 경우 NavLink의 목적과 부합하지 않으니 Link 태그를 사용하시는 것이 더 적절합니다.
| if (!tags.includes(tagInput.trim())) { | ||
| setTags([...tags, tagInput.trim()]); | ||
| } | ||
| setTagInput(""); |
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.
💊 제안
중복되는 태그가 생성되지 않도록 해주신 점 좋습니다!
다만 이렇게 되면 사용자가 해당 동작에 대한 피드백을 받지 못하므로 왜 태그가 추가되지 않고 인풋값은 초기화되는지에 대해 알 수가 없습니다. 이럴때 alert, toast, input error 와 같은 방식으로 피드백을 주시면 더 좋을 것 같아요.
|
|
||
| const selectedLabel = options.find((opt) => opt.value === value)?.label || ""; | ||
|
|
||
| const isMobile = window.innerWidth <= 767; |
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.
💊 제안
window.innerWidth <= 767 로 모바일 여부를 판단하는 로직이 여러 컴포넌트에 중복되어 있어요.
이 부분을 useIsMobile 같은 커스텀 훅으로 분리하시면 모바일 기준이 바뀔 때 한 곳만 수정하면 돼 유지보수에 더 유리합니다~
| <li | ||
| key={option.value} | ||
| onClick={() => { | ||
| onSelect(option.value); | ||
| setIsOpen(false); | ||
| }} | ||
| > | ||
| {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} | |
| onClick={() => { | |
| onSelect(option.value); | |
| setIsOpen(false); | |
| }} | |
| > | |
| {option.label} | |
| </li> | |
| <li key={option.value}> | |
| <button type="button" onClick={ ... }>{option.label}</button> | |
| </li> |
스프린트 미션 6
[기본 요구사항]
[심화 요구사항]
주요 변경사항 (미션5 에서 피드백 주신 사항)
App.jsx
pages/Item.jsx
BestProducts.jsx
ProductList.jsx
Pagination.jsx
배포 사이트
https://hyunbara6.netlify.app/
멘토에게