-
Notifications
You must be signed in to change notification settings - Fork 38
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
[2단계 - 상품 목록] 초코(강다빈) 미션 제출합니다. #77
Conversation
( 내일 이어서 리뷰 드릴게요! 내일 일정이 조금 빠듯해서 시간은 조금 늦어질 수 있을 것 같습니다 🙏 그 전에 브랜치 정리는 미리 진행해주셔도 좋을 것 같아요. ) |
src/App.tsx
Outdated
<> | ||
<Header /> | ||
<ToastNotification /> | ||
<ProductSection /> | ||
</CartProvider> | ||
</> |
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.
에러가 발생하면 context API를 활용하여 ToastNotification 컴포넌트를 띄우도록 했는데, 쿼리 각각의 isError나 onError를 활용하여 수정하려고 합니다! 그런데 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.
queryCache: new QueryCache({
onError: (error) => {
showToast(error.message);
},
}),
context API를 활용한 에러 관리에서 query를 활용한 에러 관리로 변경했습니다! 이렇게 수정한 게 맞을까요??
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.
retry 3회 후(+ 이때는 Loading UI도 잘 표시되고) 에러 발생 시 토스트도 잘 표시되는 것으로 확인했습니당 👍
Screen.Recording.2024-06-12.at.9.54.10.PM.mov
{!isError && | ||
!isLoading && | ||
cartItems && |
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.
&&
가 조금 많아 보여서 혹시 33 line 위치에 early return 처리하면 어떨까요?
// e.g.
if (isLoading) {
return null
}
return 정상컴포넌트
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.
초코, 안녕하세요 :)
2단계 작업 잘봤습니다.
리뷰가 늦어졌는데 기다려주셔 감사합니다 🙌 시간이 정말 타이트했는데 2단계 요구사항 구현 및 개선작업까지 적극적으로 진행해주신 것으로 확인했어요 💯 에러 핸들링, api 호출 최적화, UX 고려까지 다양한 방면으로 고민해주시는 모습도 너무 좋았습니다.
코드 보면서 생각나는 것들 코멘트 남겨보았는데 천천히 확인해보시고, 반영 완료되시면 리뷰 재요청 부탁드릴게요 :) 수고하셨습니당 🍫🍫🍫🍫🍫
안녕하세요 하루, 초코🍫입니다. 💻 로컬 실행
🔗 배포 링크✅ 주요 변경 사항 |
return ( | ||
<BaseButton onClick={onClick}> | ||
<BaseButton onClick={onClick} ariaLabel="상품 담기 버튼"> |
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 src = type === COUNTER_BUTTON_TYPES.INCREMENT ? PlusIcon : MinusIcon; | ||
const ariaLabel = type === COUNTER_BUTTON_TYPES.INCREMENT ? "증가 버튼" : "감소 버튼"; |
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 rootElement = document.getElementById("root"); | ||
const rootWidth = rootElement ? window.getComputedStyle(rootElement).width : "430px"; | ||
const rootWidth = window.getComputedStyle(rootElement!).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.
반영 감사합니다 :) ( 제가 제안드리긴 했지만 non-null assertion은 정말로 단언할 수 있을 때만 유의해서 사용해야합니다! )
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.
주의하면서 사용해야겠군요!
@@ -1,6 +1,6 @@ | |||
import { PlusShoppingCartIcon } from "../../assets"; | |||
import * as S from "./AddToCartButton.styled"; |
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.
역시 사소한데, 커밋 프리픽스의 style:
은 세미콜론 등 코드 포맷팅을 의미합니다. (프론트엔드의 스타일 코드가 아닌)
style : styled.ts 파일을 * as S 로 임포트하도록 수정
이렇게 코드 컨벤션을 적용해주시는 작업의 경우 refactor:
로 작성해주셔도 좋을 것 같네용 :)
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.
커밋 프리픽스가 너무 헷갈리네요,,ㅎ 주의하겠습니다
page = 0, | ||
size = 20, | ||
sort = [], | ||
category = "전체", |
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.
기본값이 있다면 interface에서 각각을 optional로 정의해도 좋겠습니다 :)
또, TSDoc 문법에 따라 defaultValue를 명시해서, 함수를 사용하는 쪽에서 hover 만으로 힌트를 받도록 해도 좋겠습니당
interface getProductsProps {
/** @defaultValue 0 */
page?: number;
/** @defaultValue 20 */
size?: number;
/** @defaultValue [] */
sort?: string[];
/** @defaultValue '전체' */
category?: string;
}

useEffect(() => { | ||
fetchProducts(); | ||
}, [setErrorStatus, page, sortOption, category]); | ||
const { data, error, fetchNextPage, hasNextPage, isFetching, refetch } = useInfiniteQuery({ |
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 handleDelete = async (id: number) => { | ||
removeItem(id); | ||
const handleDelete = async (cartItemId: number) => { | ||
removeItem({ cartItemId }); |
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.
안녕하세요 하루, 초코🍫입니다.
지난 미션 1단계에서 오류해결을 위한 힌트가 아주 도움이 많이 되었습니다.
포기하지 않고 끝까지 리뷰해주셔서 감사합니다 🙇♀️
💻 로컬 실행
🔗 배포 링크
바로가기
🔑 키워드 및 학습 목표
😮💨 상황공유
이번 미션은 react query를 사용하도록 리팩터링하는 것이었지만, step1 진행이 느렸던 관계로 아직 구현 중입니다!
처음 사용해 보는 것이라 더욱 속도가 안 나는 것 같은데요, 리팩터링 단계에서 같이 올릴 수 있도록 해보겠습니다
+) 추가로 페이지 확인할 때 chomre의 설정에서 안전하지 않은 콘텐츠를 허용으로 바꿔줘야 제대로 보인다고 합니다!
❓ 질문사항