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

[장바구니 협업 Step 2] 윤생(이윤성) 미션 제출합니다. #151

Merged
merged 72 commits into from
Jun 9, 2023

Conversation

2yunseong
Copy link

@2yunseong 2yunseong commented Jun 5, 2023

안녕하세요 브콜! 윤생입니다.

이번 미션은 백엔드와 처음 진행해보는 협업 미션이였는데요! 서로 함께 api 명세를 맞추어보고 개발을 진행하는게 뜻깊은 경험이였습니다!!
특히 쿠폰 api 명세를 서로 논의를 마친 이후, 백엔드에서 개발이 완료 되었다고 함과 동시에 서버 url만 바꿔끼웠는데 문제없이 바로 연동이 되어
짜릿함과 msw의 위엄을 느끼게 되었습니다..ㅎㅎ

재화 정책 - 쿠폰

step2의 요구사항으로 쿠폰을 사용하였는데요, 저희 팀은 아래와 같이 쿠폰 정책을 구성하였습니다.

쿠폰 종류

  • 쿠폰은 전체 금액에 적용되는 쿠폰과, 개별 상품에 적용되는 쿠폰 두 종류가 있다.
  • 각각의 쿠폰은 퍼센트(%)로 할인하거나 고정된 가격을 할인한다.

정리하자면 4가지의 쿠폰 종류가 있다. (이하 종류)

  1. 전체 금액에서 특정 퍼센트를 할인하는 쿠폰
  2. 전체 금액에서 특정 금액을 할인하는 쿠폰
  3. 개별 상품에서 특정 퍼센트를 할인하는 쿠폰
  4. 개별 상품에서 특정 금액을 할인하는 쿠폰

쿠폰 사용방법

쿠폰은 다음과 같이 사용할 수 있다.

  • 쿠폰은 전체 금액의 적용 가능한 쿠폰과 개별 금액을 적용할 수 있는 쿠폰 중 한가지 종류만 사용된다.
    • 따라서, 한 상품에 적용되는 쿠폰은 오로지 1가지다.
  • 쿠폰은 구매하려고 체크박스로 선택된 아이템에만 적용이 된다.

몇 가지 예시

  • 전체쿠폰(종류 1, 2) 사용시 상품에 개별쿠폰(종류3, 4)적용 불가능
image
  • 상품 1, 상품 2에 각각 상품에 해당하는 개별 쿠폰(종류 3,4) 적용할지 선택 가능
    • 적용 시 전체쿠폰(종류1, 2) 적용 불가능
image

실행방법

실제 서버 연결은 production 모드에서 적용해야겠지만 배포를 하지 않는관계로 development 환경에서 msw 대신 직접 배포된 서버를 사용하고 있는점 양해 부탁드립니다!

npm install
npm start

미션을 돌아보며..

미션을 돌아보며 남긴 글입니다. 한번 읽어보시고 해주고 싶은 말씀이 있으시다면.. 환영합니다 :)

  • 테스트가 굉장히 미흡하다고 생각합니다. 솔직히 말하면 step2 를 진행하면서 어떤 테스트도 진행하지 않았어요 ㅠ 변명을 하자면 기한을 맞추는데 급급해 기능요구사항을 충족시키는 대에만 신경을 썼던 것 같습니다. 이번에 테스트를 작성하지 않으면서 느낀점은 저는 테스트를 리팩터링을 하고나서 잘 동작하는지 확인하기 위해 테스트를 사용하는데, 그런 부분을 다 수동으로하고, 복잡한 쿠폰 로직에 대한 테스트를 로컬에서 수동으로 해 시이 더 걸렸던 것 같습니다.

  • 컴포넌트가 중복되는 코드가 많다고 생각합니다. 특히 주문 페이지에서요. 주문 조회와 주문 상세가 보면 똑같은 테이블 UI인데 중복되어 작성된 것을 볼 수 있습니다. 컴포넌트 분리를 잘하지 못해 아쉽습니다 흑흑

  • 리코일을 사용해 컴포넌트가 이 데이터가 동기인지, 비동기인지 알 필요없이 사용할 수 있다는 점과 useState를 쓰는것처럼 편리하게 사용할 수 있는 점은 매우 좋은 장점으로 느꼈습니다. 단점은 하나 느꼈다면 POST 나 PATCH 등에 따라 서버에 update해야하는 데이터를 다시 불러올 때? 그 때는 조금 불편함을 느꼈던 것 같습니다.

woo-jk added a commit to woo-jk/react-shopping-cart-prod that referenced this pull request Jun 5, 2023
* feat: add contents

* fix typos

* feat: remove requirements

* [장바구니 미션 Step 1] 룩소(우정균) 미션 제출합니다. (woowacourse#151)

* chore: 프로젝트 초기 세팅

* docs: 요구사항 목록 작성

* chore: eslint, prettier 설정 파일 작성

* feat: CSS reset을 위한 GlobalStyle 작성

* test: 스토리북에 CSS reset 추가

* feat: Header 컴포넌트 및 스토리 작성

* refactor: Header 컴포넌트, 스토리를 Header 폴더 내부로 이동

* feat: ProductItem 컴포넌트 및 스토리 작성

* refactor: CSS reset ul, li 스타일 수정

* feat: mock data 파일 작성

* feat: 상품 정보를 위한 Product 타입 작성

* feat: ProductList 컴포넌트, 스토리 작성

* feat: Modal 컴포넌트 및 스토리 작성

* refactor: Modal 컴포넌트 제거

* feat: Counter 컴포넌트 및 스토리 작성

* feat: ProductItem 컴포넌트에 Counter 추가

* refactor: 숫자를 가격 형식으로 변환하는 함수 분리

* feat: ProductImage 컴포넌트 및 스토리 작성

* feat: Header에 장바구니 수량 표시 추가

* refactor: mockData.json을 다양한 데이터로 수정

* refactor: ProductList 스타일 수정

* refactor: Counter 스타일 수정

* chore: eslint 설정 파일 수정

* chore: Recoil 설치

* feat: Recoil selector로 mock data를 fetch하여 상품 목록 렌더링 구현

* feat: 장바구니에 상품 추가 기능 구현

* feat: 장바구니 상품 수량 업데이트 기능 구현

* feat: 장바구니 상품 삭제 기능 구현

* feat: 장바구니 상태를 localStorage와 동기화하는 기능 구현

* refactor: 스토리북 기본 스토리 파일 제거

* feat: react-router-dom Router 추가

* refactor: 숫자임을 판단하는 함수를 별도의 모듈로 분리

* refactor: 장바구니 관련 도메인 로직을 useCartService 커스텀 훅으로 분리

* refactor: Counter 관련 로직을 useCounter 커스텀 훅으로 분리

* refactor: 장바구니 내의 상품 수량을 Recoil selector를 사용하도록 수정

* refactor: input event handler에서 숫자로 변환 후 useCounter에 전달하도록 수정

* docs: 구현 완료된 요구사항 체크

* refactor: handleCountChange -> updateCount로 함수명 수정

* refactor: 불필요한 index.css 제거

* feat: storybook RecoilRoot 데코레이터 추가

* refactor: 카운터 더하기, 빼기 아이콘 size prop 제거

* refactor: CartProduct -> CartItem으로 네이밍 수정

* refactor: 불필요한 파일 제거

* feat: storybook Counter의 숫자 업데이트를 위한 state, updater 추가

* refactor: Counter 컴포넌트 사이즈 수정

* refactor: Header 컴포넌트 position fixed로 수정

* refactor: ProductItem 컴포넌트에서 ProductImage 컴포넌트를 사용하도록 수정

* refactor: ProductImage 크기 조절을 위해 switch 대신 객체를 사용하도록 수정

* refactor: 장바구니 수량 업데이트 함수의 파라미터명을 quantity로 수정

* refactor: ProductItem 스타일 수정

* refactor: App 레이아웃 스타일 수정

* refactor: Recoil key camel case로 수정 및 비동기 에러 처리 추가

* chore: package.json에 homepage 프로퍼티 추가

* chore: 사이트 배포용 설정

* chore: 배포 homepage 경로 변경

* chore: 스토리북 배포 자동화

* docs: README 업데이트

* docs: README 개행 수정

* docs: README 스토리북 링크 수정

* refactor: useCounter 최솟값, 최댓값 인자로 받도록 수정

* refactor: useCartService 함수명 변경 및 새로운 cartItem 생성하는 함수 구현

* refactor: useFetchProductList 구현

* refactor: useFetchProductList 훅을 이용해 상품 가져오도록 변경

* refactor: 상품 종류 fetch 해오는 selector 삭제

* refactor: recoil atoms 및 selectors 폴더 구조 변경 및 파일 분리

* refactor: 사용하지 않는 라이브러리 삭제

* refactor: icons 파일 분리

* refactor: 불필요한 Number 함수 호출 제거

* feat: ProductImage 이미지 비율 속성 추가

* refactor: 장바구니 상품 총 갯수 -> 장바구니 품목 수 변경 및 전역 상태 제거

* refactor: 장바구니 개수 뱃지 항상 보이도록 변경

* chore: node 버전 업그레이드 후 storybook 라이브러리 재설치

* refactor: useCartService 훅 updateCartItemQuantity 함수 인자 커링으로 수정

* refactor: Counter 컴포넌트 로직 변경

* test: Counter 컴포넌트 스토리북 수정

* refactor: ProductItem 컴포넌트 로직 수정

* refactor: ProductItem 컴포넌트 import 순서 변경

* refactor: useFetchProductList 훅 fetchProductList 함수도 반환하도록 추가

* refactor: isNumber 함수 isNumericString 으로 네이밍 변경

* refactor: useCounterHandler 훅 중복되는 연산 로직을 변수로 선언

* refactor: useCounterHandler 파라미터 객체 형식으로 받도록 수정

---------

Co-authored-by: Suyoung <jsy8492@gmail.com>

* [장바구니 미션 Step 2] 룩소(우정균) 미션 제출합니다. (#237)

* refactor: 각 페이지 별 컴포넌트 생성 후 적용

* feat: Header 호출 위치 변경 및 버튼 클릭 시 navigate 기능 추가

* refactor: component 폴더 구조 생성

* feat: storybook MemoryRouter 추가

* refactor: pages 폴더 구조 변경

* feat: 휴지통 아이콘 svg 생성

* feat: CheckBox 컴포넌트 생성

* feat: 장바구니 페이지 레이아웃 설정

* refactor: ProductImage를 Image로 변경 후 폴더 위치 변경

* fix: CheckBox 컴포넌트 labelText 없을때는 렌더링 안되도록 수정

* refactor: CartItem 타입명 변경(CartProduct)

* refactor: Image 스토리북 폴더 위치 변경

* fix: 체크박스 컨테이너 위치 수정

* feat: CartItem 컴포넌트 구현 및 스토리북 작성

* refactor: cart 폴더 구조 생성

* feat: CartList 컴포넌트 퍼블리싱 완료

* feat: 결제예상금액 컴포넌트 퍼블리싱 완료

* feat: 결제예상금액 금액 포맷팅

* refactor: cartList 가져올 때 useCartService 훅 사용하도록 변경

* feat: 장바구니 삭제 버튼 기능 구현 및 useCartService 함수명 remove -> delete로 변경

* feat: 장바구니 삭제 버튼 기능 구현

* refactor: useCartService 업데이트, 삭제 함수 cartId로 받도록 변경

* feat: 장바구니 선택 목록 컨텍스트 Provider 생성

* feat: 장바구니 선택 목록 훅 useCheckedCartList 구현

* refactor: usePaymentAmount 훅 분리

* refactor: CheckBox 컴포넌트 프롭스 로직 변경

* refactor: CheckBox 컴포넌트 사용하지 않는 import 제거

* refactor: CartPages 컴포넌트 컨텍스트 Provider 적용

* feat: CartList 전체 선택, 선택 삭제 기능 구현

* feat: ProductItem cartId 사용하도록 로직 변경

* refactor: CartItem 컨텍스트 value 쓰도록 변경

* feat: 장바구니에 상품이 없을 때 화면 구현

* feat: 장바구니 페이지 반응형 구현

* chore: msw 라이브러리 설치

* refactor: CartItem 컴포넌트 변수 선언 위치 변경

* test: CartList 컴포넌트 스토리북 테스트 컨텍스트 적용 및 기본 장바구니 상태 적용

* test: PaymentAmount 컴포넌트 스토리북 테스트 컨텍스트 적용 및 기본 장바구니 상태 적용

* test: CartItem 컴포넌트 스토리북 테스트 컨텍스트 적용

* chore: 배포 자동화 yml 브랜치 변경

* feat: MSW 상품 조회, 장바구니 조회, 장바구니 추가 mock api 구현

* feat: MSW 장바구니 수량 변경, 장바구니 삭제  mock api 구현

* feat: MSW 세팅 및 상품 목록 get 요청 연동

* feat: MSW 장바구니 아이템 get 요청 연동

* feat: useFetch 훅 구현

* refactor: useFetch 훅 수정 및 상품 조회 api에 적용

* refactor: useFetch 훅 loading -> isLoading 네이밍 변경 및 상태 코드 대신 isError 반환하도록 변경

* refactor: useFetch 훅 로직 간단화 및 useGet으로 변경, hooks api 폴더 생성

* refactor: useGet ProductList 컴포넌트 수정

* refactor: ProductItem 컴포넌트 로직 수정(Counter 관련)

* refactor: useCartService 훅 MSW api 사용하도록 모두 변경

* refactor: useCartService 훅 함수들 async await 사용하도록 변경

* refactor: ProductList Suspense 적용

* refactor: 상품 목록 GET api 지연시간 3초 설정

* refactor: useGet->useFetch 네이밍 변경 및 Suspense 설정 적용(fetch 완료 전까지 Promise throw)

* refactor: ProductList 컴포넌트 useFetch 적용

* refactor: ProductList 스토리북 수정

* chore: storybook msw addon 설치 및 환경설정

* test: CartItem 스토리북 MSW 적용

* test: Header, ProductItem, ProductList 스토리북 MSW 적용

* chore: 배포 상태 확인

* chore: Suspense 복구

* fix: github 폴더명 대소문자 인식 못하는 문제 수정

* fix: Product 폴더 삭제

* fix: homepage 필드 제거

* chore: 원격 저장소로부터 pull

* fix: Product 다시 생긴 문제 수정

* chore: package.json hompage 필드 생성

* chore: 장바구니 fetching 에러 부분 주석

* fix: MSW worker 설정

* fix: pathname 수정

* fix: worker.start 수정

* fix: Router 설정 및 선언 위치 변경

* refactor: api url 상수화 및 적용

* feat: 상품 스켈레톤 구현 및 적용

* test: 스켈레톤 스토리북 작성

* refactor: response 관련 주석 제거

* feat: MSW api 로컬스토리지 연동

* feat: ErrorBoundary 컴포넌트 생성

* refactor: ErrorBoundary 에러 메시지 포함하도록 수정

* feat: useFetch 훅 error 상태 및 에러 try-catch 로직 추가

* feat: ProductList 컴포넌트 에러 발생시 throw 하도록 변경

* feat: ProductListPage ErrorBoundary 추가

* docs: README 초안 작성

* docs: 요구 사항 및 추가 구현 목표 정리

* refactor: imageSrc 타입 imageUrl로 네이밍 변경

* chore: gitignore .env 추가

* feat: 서버별 url 상수 및 유저 토큰 상수 작성

* feat: 서버 이름 recoil 전역 상태 및 url selector 구현

* feat: Header 서버 선택 기능 구현

* feat: 기본 서버 설정

* refactor: 서버 url 가져오는 selector를 유틸 함수로 변경

* refactor: 서버 이름 타입 체크 함수 구현

* fix: 서버 변경 시 Suspense 안되는 오류 수정

* refactor: 서버 url 가져오는 방식 변경

* refactor: cart-items 요청 인증 정보 추가

* refactor: MSW url 상수 추가

* refactor: Header select value 추가

* refactor: MSW 비활성화

* feat: CartItem 반응형 구현

* refactor: 장바구니 fetch 로딩 전역 상태 추가 및 적용

* feat: LoadingSpinner 컴포넌트 구현 및 장바구니 페이지 적용

* feat: Header 컴포넌트 반응형 구현

* refactor: cartState 초기 fetch 로직 변경

* fix: 서버 변경 시 체크 리스트 변경 안되는 문제 수정

* fix: 서버 변경 시 체크 리스트 변경 안되는 문제 수정

* chore: 배포 자동화 브랜치 설정

* docs: 요구 사항 정리 체크리스트  변경

* chore: 배포 설정 env 추가

* docs: README 수정

---------

Co-authored-by: devJang <devjang2016@gmail.com>
Co-authored-by: Suyoung <jsy8492@gmail.com>
Copy link
Author

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

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

질문을 조금 남겨보았습니다!

src/atoms/cart.ts Show resolved Hide resolved
src/atoms/cart.ts Show resolved Hide resolved
src/atoms/cart.ts Outdated Show resolved Hide resolved
Comment on lines 86 to 89
const applyCoupon = allCoupons.find(
(coupon) => coupon.id === selectedCoupons[0]
);
if (!applyCoupon) return totalDiscountPrice;
Copy link
Author

Choose a reason for hiding this comment

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

(질문)
추가로 if문으로 서버에서 받아온 유저의 전체 적용 쿠폰 중 유저가 선택한 쿠폰이 존재하는지 확인해서, 해당 로직에서 find는 보장할 수 있다고 생각합니다.
하지만 TS로 인해 조금 의도에 맞지 않는 타입가드를 작성하였는데요, (에러 회피용)
이런 경우를 어떻게 줄일 수 있을까요?

Choose a reason for hiding this comment

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

includes와 coupon.id === selectedCoupons[0]는 다른 조건 아닌가요?
사용처에서 selectedCoupons를 어떻게 관리하는지는 모르겠지만 사용처를 '몰라야 하는' 이 로직 입장에서는 합리적인 타입에러라고 생각합니다.

추가로 applyCoupon의 존재가 분기의 기준이 되는 거라면 처음부터 find로 applyCoupon을 찾아놓고 분기해도 될 것 같습니다.

const appliedCallCoupon = allCoupons.find(
  (coupon) => selectedCoupons.includes(coupon.id)
)
if (appliedCallCoupon) {
  return getDiscountPrice(appliedCallCoupon, totalPrice)
}
 ... 

Choose a reason for hiding this comment

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

allCoupon을 유일하게 하나만 적용하는 것이 기획인지도 궁금하네요.
그렇다면 왜 배열로 관리하는지도 궁금하구요~

Copy link
Author

Choose a reason for hiding this comment

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

api 명세에서 선택된 쿠폰을 배열로 담아 보냅니다. (selectedCoupon)
allCoupon이 하나만 적용된다면, 나머지 쿠폰은 적용할 수 없기 때문입니다.
specificCoupon은 여러 개가 적용하므로, selectedCoupon에 배열로 담깁니다.

주문 요청 API

주문할 물건과 couponIds를 함께 보냅니다.

Request method:	POST
Request URI:/orders
Headers:Authorization=Basic {Base64 Encoding}
Content-Type=application/json
Body:
{
    "cartItemIds": [1,2,3,4,5],
    "couponIds": [1L, 2L, 3L]
}

src/atoms/cart.ts Show resolved Hide resolved
src/hooks/useAllCouponSelect.ts Outdated Show resolved Hide resolved
src/mocks/api.ts Outdated Show resolved Hide resolved
Copy link
Author

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

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

질문을 조금 남겨보았습니다!

@woowahan-cron
Copy link

타이어보다 싸서 얼마인가요? 파이팅.

Copy link

@Tanney-102 Tanney-102 left a comment

Choose a reason for hiding this comment

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

안녕하세요 윤생!
레벨2 마지막 미션 정말 고생 많으셨습니다~
쿠폰 정책이 꽤 복잡해 보이는데 충분히 잘 해결해주신 것 같습니다!
몇몇 코멘트와 더불어 코드 레벨에서 주신 질문에도 답변 달아두었으니 확인 부탁드려요.

테스트가 굉장히 미흡하다고 생각합니다. 솔직히 말하면 step2 를 진행하면서 어떤 테스트도 진행하지 않았어요 ㅠ 변명을 하자면 기한을 맞추는데 급급해 기능요구사항을 충족시키는 대에만 신경을 썼던 것 같습니다. 이번에 테스트를 작성하지 않으면서 느낀점은 저는 테스트를 리팩터링을 하고나서 잘 동작하는지 확인하기 위해 테스트를 사용하는데, 그런 부분을 다 수동으로하고, 복잡한 쿠폰 로직에 대한 테스트를 로컬에서 수동으로 해 시이 더 걸렸던 것 같습니다.

당장 기능을 개발하는 게 급한 상황에서는 많은 경우 테스트에 소흘해지게 되는 것 같아요~
요건 저희 팀에서 늘 겪는 문제입니다🥲
이번 미션을 통해 테스트의 역할을 충분히 느끼셨다면 그 자체로 좋은 경험이 되었으리라 생각 되네요~

컴포넌트가 중복되는 코드가 많다고 생각합니다. 특히 주문 페이지에서요. 주문 조회와 주문 상세가 보면 똑같은 테이블 UI인데 중복되어 작성된 것을 볼 수 있습니다. 컴포넌트 분리를 잘하지 못해 아쉽습니다 흑흑

크게 문제삼을만한 반복은 아니라고 생각합니다.
그럼에도 아쉬움이 남는다면 유연한 공통 컴포넌트를 늘려 이를 해결해볼 수 있을지 고민해보셔도 좋겠네요~

리코일을 사용해 컴포넌트가 이 데이터가 동기인지, 비동기인지 알 필요없이 사용할 수 있다는 점과 useState를 쓰는것처럼 편리하게 사용할 수 있는 점은 매우 좋은 장점으로 느꼈습니다. 단점은 하나 느꼈다면 POST 나 PATCH 등에 따라 서버에 update해야하는 데이터를 다시 불러올 때? 그 때는 조금 불편함을 느꼈던 것 같습니다.

꼭 리코일이 아니더라도 mutation 후 서버 상태와 동기화 하는 과정은 필요하긴 합니다~
요걸 더 선언적으로 할 수 있도록 하는 라이브러리가 있는지 찾아보셔도 좋을 것 같네요

src/utils/debounce.ts Show resolved Hide resolved
src/pages/OrderPage/OrderPage.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
import styled from 'styled-components';

export const Root = styled.main``;

Choose a reason for hiding this comment

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

이 컴포넌트는 왜 styled component로 선언되어야하나요?

Copy link
Author

Choose a reason for hiding this comment

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

기존 코드 스타일을 따라가려고 사용하는 스타일이 없음에도 불구하고 스타일 컴포넌트를 생성하였습니다.

Choose a reason for hiding this comment

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

그럼 모든 마크업 요소가 Styled component로만 작성되어야 하나요~?
컨벤션이라면 어쩔 수 없지만 저희 팀 코드였다면 저는 그 컨벤션에 반대했을 것 같아요!
근거가 '코드 스타일' 하나 뿐인 것치고 개발 생산성에 그리 도움되지 않는 것 같습니다.
차라리 사용처에서 그냥 main을 썼으면 시멘틱 태그가 쉽게 파악되어 더 좋았을 것 같아요

},
});

export const selectedItemsState = atom({
key: 'selectedItemsState',
// TODO: 아래 두 selector, atom 중복됨

Choose a reason for hiding this comment

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

이 TODO는 해결 예정이신가요?

Copy link
Author

Choose a reason for hiding this comment

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

넵! 이번 리뷰에 반영하겠습니다.

Comment on lines 86 to 89
const applyCoupon = allCoupons.find(
(coupon) => coupon.id === selectedCoupons[0]
);
if (!applyCoupon) return totalDiscountPrice;

Choose a reason for hiding this comment

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

includes와 coupon.id === selectedCoupons[0]는 다른 조건 아닌가요?
사용처에서 selectedCoupons를 어떻게 관리하는지는 모르겠지만 사용처를 '몰라야 하는' 이 로직 입장에서는 합리적인 타입에러라고 생각합니다.

추가로 applyCoupon의 존재가 분기의 기준이 되는 거라면 처음부터 find로 applyCoupon을 찾아놓고 분기해도 될 것 같습니다.

const appliedCallCoupon = allCoupons.find(
  (coupon) => selectedCoupons.includes(coupon.id)
)
if (appliedCallCoupon) {
  return getDiscountPrice(appliedCallCoupon, totalPrice)
}
 ... 

Comment on lines 86 to 89
const applyCoupon = allCoupons.find(
(coupon) => coupon.id === selectedCoupons[0]
);
if (!applyCoupon) return totalDiscountPrice;

Choose a reason for hiding this comment

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

allCoupon을 유일하게 하나만 적용하는 것이 기획인지도 궁금하네요.
그렇다면 왜 배열로 관리하는지도 궁금하구요~

return totalDiscountPrice;
}

selectedCoupons.forEach((couponId) => {

Choose a reason for hiding this comment

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

reduce를 사용하셔도 될 것 같네요.
totalDistancePrice라는 가변변수는 굳이 필요 없어 보입니다.

src/atoms/cart.ts Show resolved Hide resolved
src/hooks/useAllCouponSelect.ts Outdated Show resolved Hide resolved
Copy link
Author

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

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

브콜 안녕하세요! 리뷰 반영하고 다시 요청 드립니다!
혹시 또 부족한 부분 있으시다면 코멘트 부탁드립니다~! 감사합니다 👍

src/pages/OrderPage/OrderPage.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
import styled from 'styled-components';

export const Root = styled.main``;
Copy link
Author

Choose a reason for hiding this comment

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

기존 코드 스타일을 따라가려고 사용하는 스타일이 없음에도 불구하고 스타일 컴포넌트를 생성하였습니다.

Comment on lines 86 to 89
const applyCoupon = allCoupons.find(
(coupon) => coupon.id === selectedCoupons[0]
);
if (!applyCoupon) return totalDiscountPrice;
Copy link
Author

Choose a reason for hiding this comment

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

api 명세에서 선택된 쿠폰을 배열로 담아 보냅니다. (selectedCoupon)
allCoupon이 하나만 적용된다면, 나머지 쿠폰은 적용할 수 없기 때문입니다.
specificCoupon은 여러 개가 적용하므로, selectedCoupon에 배열로 담깁니다.

주문 요청 API

주문할 물건과 couponIds를 함께 보냅니다.

Request method:	POST
Request URI:/orders
Headers:Authorization=Basic {Base64 Encoding}
Content-Type=application/json
Body:
{
    "cartItemIds": [1,2,3,4,5],
    "couponIds": [1L, 2L, 3L]
}


if (allCoupons.some((coupon) => selectedCoupons.includes(coupon.id))) {
const applyCoupon = allCoupons.find(
(coupon) => coupon.id === selectedCoupons[0]
Copy link
Author

Choose a reason for hiding this comment

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

allCoupon임을 처음에 if문(85번째줄)에서 보장한다고 생각해
도메인 상 선택 로직에서 하나만 선택할 수 있도록 보장하므로 충분하다고 생각합니다.

},
});

export const selectedItemsState = atom({
key: 'selectedItemsState',
// TODO: 아래 두 selector, atom 중복됨
Copy link
Author

Choose a reason for hiding this comment

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

넵! 이번 리뷰에 반영하겠습니다.

Copy link

@Tanney-102 Tanney-102 left a comment

Choose a reason for hiding this comment

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

안녕하세요 윤생~
긴 시간 정말 고생 많으셨습니다.
이제 우테코에서 리뷰를 받는 미션은 이걸로 끝이군요!
부디 이 과정이 윤생의 성장의 크게 도움이 되었기를 바랍니다! 또 다음 레벨에 준비되어 있는 과정들이 더 큰 성장의 재료가 되기를 기대합니다!
정말 고생 많으셨습니다!

코멘트에 리플라이 몇개 달아두었으니 그것만 확인하고 푹 쉬시면 될 것 같네요~

@Tanney-102 Tanney-102 merged commit 3267276 into woowacourse:2yunseong Jun 9, 2023
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.

3 participants