-
Notifications
You must be signed in to change notification settings - Fork 24
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
[김현묵] sprint 5 #97
The head ref may contain hidden characters: "basic-\uAE40\uD604\uBB35-sprint5"
[김현묵] sprint 5 #97
Conversation
Basic 김현묵 sprint1,2
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.
- PR CL(Change List)에 이번 스프린트 미션과는 관계 없는 파일들이 다수 보이는 것 같습니다. base branch와 target branch의 동기 지점을 고려해보면 좋을 것 같네요.
페이지 버튼 부분 저렇게 처리하는지 궁금합니다. pageNation
버튼 자체를 숨기는 방식도 유효하고, 노출하되 클릭하지 못하도록 disable 시키는 방법도 유효합니다.
usePageNavi, | ||
useScreenSize, | ||
} from "./hook/hook"; | ||
let init = { |
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로 정의해도 객체 내부 값들은 변경할 수 있기 때문에 object 자체를 변경하려는 의도가 아니라면 const로 정의해도 충분할 것 같습니다.
그리고 init이라는 이름을 보면 애초에 초기값 전달을 목표로 정의한 걸로 보이는데, 그 의도가 맞다면 더더욱 const로 선언하고, 더 나아가서는 Object.freeze 등의 method를 활용할 수도 있을 것 같습니다.
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 [_list, _setList] = useState(items); | ||
const [_length, _setLength] = useState(leng); | ||
return { | ||
value: _list, | ||
setValue: (setItems) => { | ||
_setList(setItems); | ||
}, | ||
length: _length, | ||
setLength: (setLeng) => { | ||
_setLength(setLeng); | ||
}, | ||
}; |
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.
이름에 _가 많이 포함되는 건 좋은 코드 스타일은 아닌 것 같습니다.
start, | ||
setStart: (startNum) => { | ||
setStart(startNum); | ||
}, | ||
last, | ||
setLast: (lastNum) => { | ||
setLast(lastNum); | ||
}, |
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.
setStart와 setLast 둘 다 이름 그대로 넘겨도 될 것 같네요.
const [sellLimit, setSellLimit] = useState(10); | ||
const naviLimit = 5; | ||
const bestProduct = useItmeList([], 4); | ||
const sellProduct = useItmeList([], sellLimit); | ||
const [onTarget, setOnTarget] = useState(1); | ||
const [arrType, setArrType] = useState("recent"); | ||
const [total, setTotal] = useState(0); | ||
const pageNavi = usePageNavi(1, 5); | ||
const searchRef = useRef(null); | ||
const searchHandle = useChange(); | ||
const windowSize = useScreenSize(); | ||
const [bestItmeSize, setBestItemSize] = useState("282px"); | ||
const [sellItmeSize, setSellItemSize] = useState("220px"); |
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 searchKewordHandle = (e) => { | ||
if (e.keyCode === 13 || e.type.toString() === "click") |
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.
13은 무슨 값일까요? (질문의 의도가 무엇일까요? 👀 )
sellProduct.setValue(res.list); | ||
setTotal(res.totalCount / sellLimit); |
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.
product에 따라 종속성이 생기는 total도 위 코멘트와 같습니다. 이 경우라면 state를 object 형태로 관리하는 걸 고려해볼 수 있겠습니다.
<div className="marketEtc"> | ||
<div className="searchBox"> | ||
<button className="submit" onClick={searchKewordHandle}> | ||
<img src="./img/ic_search.svg" alt="search" /> | ||
</button> | ||
<input | ||
type="text" | ||
ref={searchRef} | ||
onChange={searchHandle.onChange} | ||
onKeyDown={searchKewordHandle} | ||
className="marketSearch" | ||
placeholder="검색할 상품을 입력해주세요" | ||
/> | ||
</div> | ||
<a href="#">상품등록하기</a> | ||
<select onChange={selectHandle}> | ||
<option value="recent" defaultChecked> | ||
최신순 | ||
</option> | ||
<option value="favorite">좋아요순</option> | ||
</select> | ||
</div> | ||
</MarketSection> | ||
<MarketPageNavi | ||
start={pageNavi.start} | ||
last={pageNavi.last} | ||
target={onTarget} | ||
onClick={pageNaviEvent} | ||
onNext={nextEvent} | ||
onPrivous={privousEvent} | ||
total={total} |
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.
👍 핸들러 깔끔하게 잘 넘기셨네요.
let path = `${url}?page=${page}&pageSize=${pageSize}&orderBy=${orderBy}`; | ||
if (!!keyword) path += `&keyword=${keyword}`; |
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.
URL을 다양하게 다뤄야 한다면 스트링 값을 더하고 빼는 것 대신 URL와 URLSearchParams 내장 객체를 참고해보시면 좋을 것 같습니다.
<div className="App" style={init}> | ||
<Header /> | ||
<div className="container"> | ||
<MarketSection | ||
className={"best"} | ||
title={"베스트 상품"} | ||
data={bestProduct.value} | ||
itemMaxWidth={bestItmeSize} | ||
skeletonLength={bestProduct.length} | ||
/> | ||
<MarketSection | ||
className={"sell"} | ||
title={"판매 중인 상품"} | ||
data={sellProduct.value} | ||
itemMaxWidth={sellItmeSize} | ||
skeletonLength={sellProduct.length} | ||
> | ||
<div className="marketEtc"> | ||
<div className="searchBox"> | ||
<button className="submit" onClick={searchKewordHandle}> | ||
<img src="./img/ic_search.svg" alt="search" /> | ||
</button> | ||
<input | ||
type="text" | ||
ref={searchRef} | ||
onChange={searchHandle.onChange} | ||
onKeyDown={searchKewordHandle} | ||
className="marketSearch" | ||
placeholder="검색할 상품을 입력해주세요" | ||
/> | ||
</div> | ||
<a href="#">상품등록하기</a> | ||
<select onChange={selectHandle}> | ||
<option value="recent" defaultChecked> | ||
최신순 | ||
</option> | ||
<option value="favorite">좋아요순</option> | ||
</select> | ||
</div> | ||
</MarketSection> | ||
<MarketPageNavi | ||
start={pageNavi.start} | ||
last={pageNavi.last} | ||
target={onTarget} | ||
onClick={pageNaviEvent} | ||
onNext={nextEvent} | ||
onPrivous={privousEvent} | ||
total={total} | ||
/> | ||
</div> | ||
<Footer /> | ||
</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.
React에서 마크업을 구성할 때는 비슷한 추상화 수준을 유지시켜주면 가독성 향상에 도움이 많이 됩니다. Header와 Footer처럼 중간이 있는 div 부분도 컴포넌트로 한 번 분리되면 좋을 것 같네요.
요구사항
기본
중고마켓 페이지
심화
중고마켓 페이지
멘토에게