-
Notifications
You must be signed in to change notification settings - Fork 43
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
[최성락] Sprint7 #260
The head ref may contain hidden characters: "React-\uCD5C\uC131\uB77D-sprint7"
[최성락] Sprint7 #260
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 (error instanceof HttpException) { | ||
throw error; | ||
} else { | ||
console.error("네트워크 오류", error); | ||
throw new Error("네트워크 오류가 발생했습니다. 잠시 후 다시 시도해주세요."); | ||
} | ||
} |
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쪽에서 디버깅하기가 수월해질거에요
import ItemCard from "../ui/Item/ItemCard"; | ||
import "./AllProductsSection.css"; | ||
|
||
const getPageSize = () => { | ||
const width = window.innerWidth; | ||
const getPageLimit = (width) => { |
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 (error instanceof HttpException) { | ||
setError(error.message); | ||
} else { | ||
setError("알 수 없는 오류가 발생했습니다."); | ||
} |
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.
사실 이미 fetchproduct에서 던져지는 에러 문구 핸들링은 다 하고 있잖아요
그래서 바로 setError(error.message)로 통합해서 처리해도 괜찮아보여요
const getProducts = async (limit, sort) => { | ||
try { | ||
const { list } = await fetchProducts(sort, limit); |
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.
이 함수들처렴 인자가 여러개 들어가기 시작한다면, 인자를 객체로 전달받는 형태도 고려해보면 좋습니다
} else { | ||
setError("알 수 없는 오류가 발생했습니다."); | ||
} | ||
} | ||
}; | ||
|
||
const handleResize = useCallback(() => { |
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.
사실 콜백으로 감싸서 재평가 자체는 안되도록 할 수 있지만 실질적으로 onResizeEvent가 와르르 발생할때에 대한 퍼포먼스 핸들링은 어려울거에요.
따라서 쓰로틀링이나 디바운싱을 활용해보면 좋겠는데요
// utils.js
function debounce(callback, delay = 300) {
let timeoutId = null;
return (...args) => {
if (timeoutId) {
clearTimeout(timeoutId);
}
timeoutId = setTimeout(() => {
callback(...args);
}, delay);
};
}
로 구현해두고
useEffect(() => {
const handleResize = debounce(() => {
const newPageSize = getPageLimit(window.innerWidth);
if (newPageSize !== pageSize) {
setPageSize(newPageSize);
}
}, 300); // 300ms 지연 시간 설정
window.addEventListener('resize', handleResize);
return () => {
window.removeEventListener('resize', handleResize);
};
}, [pageSize]);
이런식으로 활용해보면 어떨까 합니다
const getRelativeTime = (createdAt, updatedAt) => { | ||
if (updatedAt && updatedAt !== createdAt) { | ||
return formatRelativeTime(updatedAt); | ||
} | ||
return formatRelativeTime(createdAt); | ||
}; |
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.
하나 의견있는데요, 지금 이 로직의 경우
- 생성일과 수정일이 동일하다면, 생성일 기준으로 날짜 파싱
- 생성일과 수정일이 다르다면 수정일 기준으로 날짜 파싱
을 하고 있잖아요?
근데 결국 위 두 로직 다 결국 수정일을 기준으로 날짜 파싱해도 동일한 결과가 나오는 것으로 생각됩니다.
애초에 디비 설계상 데이터가 최초 생성되면 수정일도 해당일과 동일하게 생성이 될거고,
특정 엔티티를 수정하면 수정일만 변경이 되다보니깐요.
만약 코드잇측에서 디비 설계를 좀 요상하게 해서 최초 생성시 수정시간이 등록되지 않는다면 지금처럼 두어도 무난하지만 그게 아니라면 updatedAt 기준만 활용해서 로직 파싱해도 될것 같습니다
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가 아닌곳에서 필요한 값을 꺼내오는 부분이라고 생각되어요.
이건 사실 개인 취향에 따라 달려있는기능이긴 한데요, 날짜 데이터의 경우, 되도록이면 UI 콤포넌트에게 날짜형데이터는 날짜형태로 전달을 해주고, 환경에 따라서 UI로 파싱하는건 UI 콤포넌트에서 util function을 활용해 풀어주는걸 선호하고 있어요!
왜냐면 나중에 콤포넌트 관리할때, 날짜를 보여주는 부분을 수정하고자 한다면 당연히 UI 콤포넌트로 먼저 이동하게 될텐데, 거기에서 이미 다 형처리가 완료된 데이터를 prop으로 전달받는다면 UI 관리 책임이 조금 어긋난다고 생각하기 때문이에요.
이거는 한번 성락님 취향에 맞춰 탐색해보시면 어떨까요?
{tags && tags.length > 0 ? ( | ||
tags.map((tag) => ( | ||
<li key={tag} className="item-detail-tag"> | ||
#{tag} | ||
</li> | ||
)) | ||
) : ( | ||
<li></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.
리스트가 없으면 아무것도 렌더링 안해주면 안되나용?
{tags && tags.length > 0 ? ( | |
tags.map((tag) => ( | |
<li key={tag} className="item-detail-tag"> | |
#{tag} | |
</li> | |
)) | |
) : ( | |
<li></li> | |
)} | |
{tags && tags.map((tag) => ( | |
<li key={tag} className="item-detail-tag"> | |
#{tag} | |
</li> | |
) | |
)} |
const formatDate = (dateString) => { | ||
const date = new Date(dateString); | ||
const year = date.getFullYear(); | ||
const month = String(date.getMonth() + 1).padStart(2, "0"); | ||
const day = String(date.getDate()).padStart(2, "0"); | ||
return `${year} ${month}.${day}`; | ||
}; |
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.
date util파일에 옮겨볼까요?
const [item, setItem] = useState(null); | ||
const [comments, setComments] = 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.
댓글과 product detail을 분리해서 훅으로 두면 어떨까요?
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 ProductDetailPage.jsx() {
const params = useParams()
const productId = params.id;
const {data: productDetail, isLoading: isProductLoading, error: productError} = useProductDetail(productId)
const { data: comments, isLoading: commentsLoading, error: commentsError } = useCommentForProduct(productId)
...
}
<div className="item-detail-page"> | ||
<Header /> | ||
<p>로딩 중...</p> | ||
</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.
이 부분들이 계속 반복되고 있죠?
그럼 렌더링 하는 곳에서 이렇게 하는게 더 수월할 수 있습니다
return <div className='item-detail-page'>
<Header />
{loading && 로딩중 }
{error && 에러 콤포넌트 }
{ !loading && !error && data && 데이터 콤포넌트 }
</div>
요구사항
기본
상품 상세
상품 문의 댓글
심화
스크린샷
멘토에게