Skip to content
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

Feat/385 #391

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

Feat/385 #391

wants to merge 9 commits into from

Conversation

devkyoung2
Copy link
Member

#️⃣연관된 이슈

close: #385

📝작업 내용

  • 장바구니 개수 카운팅 기능 구현
  • 장바구니 상품 체크/삭제시 포인터로 변경 및 이벤트 전파 범위 수정

🙏리뷰 요구사항(선택)

@devkyoung2 devkyoung2 requested review from uraflower and ohgus July 8, 2024 05:30
@devkyoung2 devkyoung2 self-assigned this Jul 8, 2024
Copy link

vercel bot commented Jul 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kakao-funding ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 8, 2024 5:30am

Comment on lines +33 to +36
{isLoggedIn && !isLoading && cartCount! > 0 && (
<span className={styles.num_cart}>{cartCount}</span>
)}
{isLoading && <Spinner />}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 카운트 표기할 때 cartCount! > 0 요 조건이 있는데, cartCount가 non-null로 단언이 되어 있는 것 같아요. cartCount는 API 응답인데 이렇게 단언해도 되나요??
  2. 로딩 중일 때 스피너가 뜨도록 되어 있는데, 스피너가 뜨면 화면 상호작용이 불가능합니다. 장바구니 아이템 개수 렌더링 같은 경우는 페이지 상호작용을 멈춰야 할 정도로 중요한 작업이 아니라고 봅니다. 그래서 스피너를 굳이 띄울 필요가 없다고 생각하는데 어떻게 생각하시나요?

Copy link
Member Author

@devkyoung2 devkyoung2 Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. API 응답이 완료되지 않으면 !isLoading에서 이미 false를 리턴하니까 cartCountnull값일 수가 없어요. 만약 API 요청에서 에러가 발생해 에러핸들링하는 경우를 말씀하시는거라면 이 컴포넌트가 아니라 인터셉트에서 핸들링하는게 낫지 않을까요?
  2. 음... 그런것 같기도 하네요... 제거하겠습니다

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 그렇겠네요 이해했습니다 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

장바구니 총 개수 조회 API 연동
2 participants