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

[1단계 - 장바구니 미션] 동동(김동희) 미션 제출합니다. #19

Merged
merged 56 commits into from
May 21, 2021

Conversation

bigsaigon333
Copy link

@bigsaigon333 bigsaigon333 commented May 13, 2021

@ysm0622 님 안녕하세요 처음 인사드립니다. 저는 동동 🏋️‍♂️ 입니다.

장바구니 미션 1단계 PR 제출하오니 리뷰 부탁드립니다!

  1. 리액트로 인해 요즘 많이 헤매고 있습니다. 최소한의 기본사항만 구현하였고, 심화사항은 여력이 된다면 2단계때 적용해 볼 계획입니다..!

  2. RTK가 Redux 도입시 Best Practice라고 공식홈페이지에 적혀있어, 별 고민없이 RTK 를 도입하였습니다. 확실히 사용법이 간편하였는데, trade-off 로 Redux에 대해 많이 고민하지 못한 것 같아 아쉬운 면도 있습니다. 2단계때 비동기 로직을 도입하며 Redux에 대해 더 고민해보도록 하겠습니다.

  3. TypeScript Migration 또는 Redux-saga 를 도입해보라는 제안을 받았습니다... 가능하면 한번 도전해보려고 합니다. 만약 그렇다면 2단계 PR 리뷰 요청때는 이전과는 사뭇 다른 코드로 크리스님을 찾아뵙게 되겠네요... 너그러이 용서해주시면 감사하겠습니다 😅

1. 🔥 데모사이트 🔥

2. ✈️ VSCODE in web ✈️

3. 구현한 기능 목록 - 1단계

  • REQUIREMENTS.md에 요구 사항 도출
  • 재사용 가능한 Component 작성
  • 장바구니
  • 주문/결제
  • 상품 목록
  • 주문 목록

hchayan and others added 30 commits May 4, 2021 15:40
Co-authored-by: Chayan <hchayan196@gmail.com>
Copy link

@ysm-dev ysm-dev left a comment

Choose a reason for hiding this comment

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

UX 관련 피드백

  • 새로고침해도 장바구니가 남아있게하려면 어떻게하는게 좋을까요?
  • https://bigsaigon333.github.io/react-shopping-cart/cart 페이지에서 새로고침하면 404가 뜨는데 어떻게 해결할 수 있을까요?
  • (개선사항)주문목록이나 장바구니에 아무것도 없을땐 빈 화면을 보여주는것보다 "주문하러 가볼까요?", "장바구니에 담아볼까요?" 문구와 함께 상품구매 페이지로 전환하는 버튼을 추가하면 좀더 구매전환률이 높아지지 않을까요? ㅎㅎ

전체적으로 코드가 너무 깔끔하고 훌륭합니다! 👍
피드백 드릴게 많지 않네요!

고생하셨습니다 👏 👏 👏 👏

Comment on lines 19 to 28
const checkedSet = new Set(
Object.values(cart).map(({ checked }) => checked)
);

if (checkedSet.size === 0) {
setCheckAll(false);
}
if (checkedSet.size === 1) {
setCheckAll([...checkedSet].pop());
}
Copy link

Choose a reason for hiding this comment

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

useEffect 쓰지 않고 좀더 선언적으로 처리할 수 있을것같은데 좀더 고민해볼까요? 👍

Copy link
Author

Choose a reason for hiding this comment

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

전체선택에 대한 UI 정의를 잘못 내렸던 것 같습니다.
'전체선택/해제 기능'과 '선택된 것만 전체해제'를 하나의 버튼으로 구현하려고 처음에 하였습니다.

이에 대한 로직이 상당히 복잡해져서, 처음으로 돌아가 전체선택 버튼에 대한 기능을 다시 생각해보았습니다.
또한, 쿠팡의 장바구니 화면의 전체선택 버튼을 참고하였습니다.

이를 바탕으로 전체선택 버튼은 전체선택 / 전체선택해제 기능만 수행하는 것으로 수정하였습니다 😄

관련 커밋입니다: d16b93e

Comment on lines 25 to 28
if (!location.state?.isAllowed) {
window.alert("비정상적인 접근입니다");
return <Redirect to="/cart" />;
}
Copy link

Choose a reason for hiding this comment

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

누군가 url에 isAllowed를 직접 쳐서 들어온다면 그건 정상적인 접근일까요? ㅎㅎ
근본적인 해결책은 무엇일까요?

Copy link
Author

Choose a reason for hiding this comment

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

route가 있으나, url을 통해서 접근하는 것은 막으려는 것이 과연 올바른 route의 사용법일까... 고민을 많이 했습니다.

접근1. "/cart" 에서 Cart 와 Payment를 모두 구현

CartRoute를 만들고, Cart와 Payment 중 어느 것을 나타낼지를 state로 저장하여, 이에 따라 렌더링 되는 컴포넌트를 달리 하였습니다.

const CartRoute = ({ exact, path }) => {
  const [readyToPay, setReadyToPay] = useState(false);

  const handleCartButtonClick = () => setReadyToPay(true);

  return (
    <Route exact={exact} path={path}>
      {readyToPay ? (
        <Payment />
      ) : (
        <Cart onButtonClick={handleCartButtonClick} />
      )}
    </Route>
  );
};

접근2. context API 활용

접근1도 뭔가 아쉽긴 하지만 나쁘지 않다고 생각했습니다. 그러던 중 authentication 기능이 있다면 모든 route에서 일어날 수 있는 상황이겠다는 생각이 들었습니다. 이에 관련 reference를 찾던 중 context API를 활용한 react-router-dom 의 Redirects (Auth) 를 발견하였습니다.
https://reactrouter.com/web/example/auth-workflow

저번 페이먼츠 미션 2단계가 contextAPI를 적용하는 것이었는데, 이리저리 리팩토링 하다가 아직 contextAPI를 적용하지 못하였습니다.. 따라서 이번에 처음 contextAPI를 적용해본 것인데, 용도에 맞게 잘 사용한 것인지 좀 의문이 듭니다...

결론적으로 접근2. context API 활용하여 리팩토링하였습니다.

@ysm0622 께서 생각하신 근본적인 해결책이 맞는지 궁금합니다. 아니라면 힌트를 조금만 주시면 더 고민해보겠습니다 😃

관련 커밋입니다: 6342098

Copy link

Choose a reason for hiding this comment

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

괜찮은 방법입니다! 👍

@bigsaigon333
Copy link
Author

새로고침해도 장바구니가 남아있게하려면 어떻게하는게 좋을까요?

redux내의 state가 갱신될 때마다 localStorage에 state를 저장하도록 하였습니다.

이를 위해 localStorage에 state를 저장하는 함수가 store를 subscribe하도록 하였으며,
너무 잦은 호출을 방지하기 위해 디바운스도 적용하였습니다.

처음에 react-redux의 hooks를 이용해서 store의 state를 갱신하는 컴포넌트를 구현할까도 생각하였습니다.
localStorage에 저장하는 행위 자체는 UI와 관련이 없으므로 굳이 react-redux의 hooks를 사용하지 않고, 직접 store에 subscribe를 하였습니다.

모든 컴포넌트는 개별적인 store의 subscriber 라는 자료를 보아서, react-redux의 hooks 사용여부는 성능면에서는 상관이 없다고 생각하였습니다.

다만 이러한 접근법이 일반적인 것인지, 제대로 된 것인지 의문이 듭니다...
(사실은 react-redux의 hooks를 사용해서는 어떻게 해야할지 잘 몰라서.... 못한 것도 있습니다 😢 )

참고한 reference는 아래와 같습니다.

관련 커밋입니다: 5154809

@bigsaigon333
Copy link
Author

https://bigsaigon333.github.io/react-shopping-cart/cart 페이지에서 새로고침하면 404가 뜨는데 어떻게 해결할 수 있을까요?

github pages 는 기본적으로 SPA만 지원하므로, "/" 가 아닌 다른 url에 접근한 경우 404를 반환합니다.

index.html 외의 404.html을 deploy한 경우, 404 를 반환하며 404.html을 표시하는데, 이를 이용하여 "/"로 리다이렉트하는 기능을 구현하였습니다.

사실 이러한 github pages의 행동은 이전부터 알고 있었는데, 이를 적용할 여유가 없다는 이유로 내내 미뤄오다가 @ysm0622 님의 리뷰로 인해 적용해보았습니다.

참고한 reference는 아래와 같으며, 복붙만 해도 동작하지만.... 그러면 너무 얻는게 없는 것 같아서 나름대로 리팩토링해보고 적용하였습니다.

관련 커밋입니다.

@bigsaigon333
Copy link
Author

(개선사항)주문목록이나 장바구니에 아무것도 없을땐 빈 화면을 보여주는것보다 "주문하러 가볼까요?", "장바구니에 담아볼까요?" 문구와 함께 상품구매 페이지로 전환하는 버튼을 추가하면 좀더 구매전환률이 높아지지 않을까요? ㅎㅎ

다른 리뷰 반영하다보니 레이아웃 구성이 가장 간단하고 재밌어서 후딱 적용했습니다... (그렇다고 쉬웠다는 얘기는 아닙니다 ㅎㅎ..)
viewport 에 따른 반응형 레이아웃 구성은 2단계 하면서 여유가 나면 적용해보도록 하겠습니다...(TS, redux-saga 적용은 이미 포기했습니다 ㅎㅎ...)

image

image

Comment on lines +24 to +25
payment.getReady();
history.push("/payment");
Copy link
Author

Choose a reason for hiding this comment

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

질문 있어요 ✋

24번: payment.getReady() 을 호출 -> setReadyToPay(true) 호출 -> ProvidePayment 의 호출을 React에 request -> isReadyToPay 가 true로 갱신
25번: "/payment"로 라우트 갱신

이렇게 되는 것으로 이해하고 있습니다. 제가 궁금한 것은 setReadyToPay(true) 호출 뒤 바로 ProvidePayment가 호출되지 않는 것으로 알고 있는데, 그러면 25번 라인이 실행될 때 isReadyToPay가 true로 아직 갱신되기 전이어서 false일수도 있지 않나 라는 것입니다.

만약 "/payment" 로 라우트가 갱신되었을때 isReadyToPay가 false라면 주문/결제 화면(Payment)가 표시되지 않아야 하는데, 여러번 새로고침해보아도 그런 경우는 발견하지 못하였습니다.

제가 React에 대한 이해가 부족한 것 같은데, setReadyToPay 는 클로저이므로 이를 실행하면 outer environment의 isReadyToPay는 즉시 갱신이 되는 것이고, 단지 이에 따른 UI update(ProvidePayment의 호출)은 React가 제어한다고 이해하는 것이 맞을까요?

React로 어찌어찌 미션을 진행하고 있긴 한데, 뭔가 solid한 이해가 부족하여 매번 답답한 마음이 많이 들고 있습니다... ㅠㅠ

Copy link

Choose a reason for hiding this comment

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

제가 궁금한 것은 setReadyToPay(true) 호출 뒤 바로 ProvidePayment가 호출되지 않는 것으로 알고 있는데
왜 그렇게 알고계셨을까요?

어떤순서로 실행되는지 중간중간 dubugger나 콘솔을 찍어보는건 어떨까요? :)

Comment on lines +9 to +23
addToCart: (state, action) => {
const { id, ...product } = action.payload.product;
if (state[id]) {
state[id].amount += 1;
} else {
state[id] = {
...product,
id,
amount: 1,
addedDate: Date.now(),
checked: true,
};
}
},

Copy link
Author

Choose a reason for hiding this comment

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

질문 있어요 ✋

가볍게 의견만 주시면 감사하겠습니다...

cart state를 object keyed by id 로 관리하고 있습니다. id를 알고 있다고 가정할 때 O(1)으로 바로 접근할 수 있다는 장점이 있어서 적용하였습니다.
2단계 API를 적용하다보니 api 요청에 따른 결과값은 array로 오고 있습니다.

  1. redux의 state 형태와 api 요청에 따른 data 형식이 달라도 되는 건가 하는 의문점이 있습니다.

  2. state가 object이다보니 Object.keys, Object.values, Object.entities 등 object의 내장 메소드를 많이 사용하게 되는데,
    오히려 잦은 object 내장 메소드 사용이 더 성능을 저하시키는건 아닌가 하는 생각도 조금 듭니다.

정답을 여쭤보는 건 아닙니다.. 참고할만한 자료나 일반적인 실무 이야기(?) 등을 조금 공유해주시면 정말 도움이 될 것 같습니다!

Copy link

Choose a reason for hiding this comment

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

  1. 같으면 좋겠지만 달라도 큰문제는 없습니다. 다른경우가 대부분입니다. 프론트에서는 서버에서 받은 데이터를 적절한 구조로 가공해고 쪼개서 보여주는게 일반적입니다.
  2. 내장메소드를 사용하면 왜 성능이 저하되는지 궁금해요 👀

Copy link

@ysm-dev ysm-dev left a comment

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.

None yet

3 participants