-
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
[박수환] sprint5 #86
The head ref may contain hidden characters: "react-\uBC15\uC218\uD658-sprint5"
[박수환] sprint5 #86
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.
커밋 단위를 '코드에 의미 있는 변화가 생기는 지점'을 기준으로 잘 게 나누어서 커밋할 수 있도록 노력해보시면 좋을 것 같습니다.
if (width >= 1230) setPageSize(10); | ||
if (width >= 801 && width < 1230) setPageSize(6); | ||
if (width < 800) setPageSize(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.
조건문 코드 스타일만 조금 수정해줘도 가독성이 개선될 것 같습니다.
|
||
function AllItem() { | ||
const [products, setProducts] = useState([]); | ||
const [option, setOption] = useState('favorite'); |
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.
enum처럼 사용되는 값이면 리터럴 값을 그대로 사용하는 것보다 const object로 정의해두고 사용하면 좋을 것 같네요.
const [tempSearch, setTempSearch] = useState(""); | ||
const [search, setSearch] = useState(""); |
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.
tempSearch와 search라는 상태가 동시에 존재한다는 게 상태 관리 차원에서 불편한 느낌이드네요.
- 정말 tempSearch라는 상태가 필요한가요?
- 진짜 필요하다면 tempSearch보다 더 적절한 이름을 짓는 게 맞을 것 같습니다.
const [totalCount, setTotalCount] = useState(0); | ||
|
||
const totalPages = Math.ceil(totalCount / pageSize); | ||
const startPage = ~~((page - 1) / 5) * 5 + 1; |
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.
- bitwise operator를 직접 사용한들 성능이 유의미하게 개선된다고 보기는 힘듭니다. 차라리 바로 윗 줄 코드처럼 Math 객체의 method를 사용해서 가독성을 높이는 게 좋아보입니다.
- / 5와 * 5는 같은 맥락으로 사용되는 값일까요? -1, +1은 0, 1 인덱스 범위 조정 차원에서 많이 쓰이는 값이기 때문에 넘어간다고 해도 5 같은 값은 상수로 정의해두고 사용하는 게 좋습니다.
const updateProducts = async () => { | ||
const axiosProducts = await getProductsList(page, search, option, pageSize); | ||
setProducts(axiosProducts.products); | ||
setTotalCount(axiosProducts.totalCount); | ||
}; |
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.
products와 totalCount는 하나의 API를 호출할 때 같이 반환되는 값인데 굳이 state를 분리할 이유가 없어 보입니다. (API 호출 시 동시에 변경되는 값이기 때문)
이 경우는 객체 자체를 state로 가져가는 쪽이 좋아 보이네요.
setPage(1); | ||
} | ||
const onKeyDown = (e) => { | ||
if (e.keyCode === 13) { |
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이 뭘까요? (질문의 의도가 무엇일까요? 👀)
(products.map((product, index) => ( | ||
<div key={index} className='All-item-list-item'> | ||
<div className='contain-img'> | ||
<img src={product.images} alt={product.name} /> | ||
</div> | ||
<div className='contain-text'> | ||
<div className='contain-text-product-des'>{product.name}</div> | ||
<div className='contain-text-price'>{product.price}원</div> | ||
<div className='contain-text-like'> | ||
<a className='like-button'> | ||
<span className="material-symbols-outlined favorite-icon"> | ||
favorite | ||
</span> | ||
</a> | ||
{product.favoriteCount}</div> | ||
</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.
products로 매핑하고 있는 부분을 컴포넌트로 분리해보시면 좋을 것 같습니다.
{startPage + 1 <= totalPages && ( | ||
<button | ||
value={startPage + 1} | ||
onClick={onClickPage} | ||
className={`buttonNum ${page === startPage + 1 ? 'active' : ''}`} | ||
> | ||
{startPage + 1} | ||
</button> | ||
)} | ||
{startPage + 2 <= totalPages && ( | ||
<button | ||
value={startPage + 2} | ||
onClick={onClickPage} | ||
className={`buttonNum ${page === startPage + 2 ? 'active' : ''}`} | ||
> | ||
{startPage + 2} | ||
</button> | ||
)} | ||
{startPage + 3 <= totalPages && ( | ||
<button | ||
value={startPage + 3} | ||
onClick={onClickPage} | ||
className={`buttonNum ${page === startPage + 3 ? 'active' : ''}`} | ||
> | ||
{startPage + 3} | ||
</button> | ||
)} | ||
{startPage + 4 <= totalPages && ( | ||
<button | ||
value={startPage + 4} | ||
onClick={onClickPage} | ||
className={`buttonNum ${page === startPage + 4 ? 'active' : ''}`} | ||
> | ||
{startPage + 4} | ||
</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.
반복문을 활용해보세요. 또는 Array.prototype.map
<div key={index} className='ItemComponent'> | ||
<div className='contain-img'> | ||
<img src={product.images} alt={product.name} /> | ||
</div> | ||
<div className='contain-text'> | ||
<div className='contain-text-product-des'>{product.name}</div> | ||
<div className='contain-text-price'>{product.price}원</div> | ||
<div className='contain-text-like'> | ||
<a className='like-button'> | ||
<span className="material-symbols-outlined favorite-icon"> | ||
favorite | ||
</span> | ||
</a> | ||
<div className='contain-text-like-fs'> | ||
{product.favoriteCount} | ||
</div> | ||
</div> | ||
</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.
이런 부분도 컴포넌트로 정의하면 좋을 것 같네요.
|
||
const getProductsList = async (page=1, search="", option="favorite", pageSize=10) => { | ||
try { | ||
const response = await axios.get(`https://panda-market-api.vercel.app/products?page=${page}&pageSize=${pageSize}&orderBy=${option}&keyword=${search}`); |
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 주소 값은 재사용되면서 환경에 따라 바뀔 수 있는 값이기에 우선은 상수로 정의해서 동일한 API 주소에 대해선 같이 사용할 수 있게 하면 좋을 것 같습니다.
요구사항
기본 요구사항
공통
배포 주소 : https://funny-shortbread-b8c7a8.netlify.app/
중고마켓 페이지
심화 요구사항
공통
중고마켓 페이지
주요 변경사항
멘토에게