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

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

Merged
merged 22 commits into from
Jun 4, 2021

Conversation

bigsaigon333
Copy link

@bigsaigon333 bigsaigon333 commented Jun 1, 2021

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

@ysm0622 님 안녕하세요. 동동 🏋️‍♂️ 입니다.

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

1. 🔥 데모사이트 🔥

2. ✈️ VSCODE in web ✈️

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

  • 제공 받은 API로 Endpoint와 Schema 변경
  • 상품 상세 페이지 구현
  • 반응형 레이아웃 적용
  • 디렉토리명 변경 (도메인명 > list > listItem)
  • 각 listItem이 store를 구독하도록 변경(리렌더링 최소화)

4. 고민한 점

  • 기존에 중첩된 형태의 디렉토리 구조를, flat한 형태로 수정하였습니다. 또한 각 route에 해당하는 컴포넌트는 pages 디렉토리로 분리하였습니다. 디렉토리 구조를 변경하고 나서 뭔가 뿌듯한 느낌이었는데, Files changed를 보니 @ysm0622 께서 리뷰하시는 데는 매우 불편해졌겠구나 라는 생각이 들었습니다.. 번거롭게 해서 죄송합니다. 처음 디렉토리 구조를 정한 후에는 쭉 갔어야 했는데, 이러한 것을 생각하지 못하고 변경해버렸습니다.. 다음부터는 주의하겠습니다. 😿
  • useSelector hook을 사용하는 각각의 컴포넌트가 store를 subscribe 한다는 것을 인지하지 못하고, 이곳 저곳에서 useSelector를 사용하여 상태의 일부분만 바뀌어도 전체 page가 리렌더링되고 있었습니다.
    이를 react-reduxshallowEqual 과 각 state의 id 배열을 잘 활용하여, 변화가 있는 컴포넌트만 렌더링되도록 하였습니다.
  • 비동기 로직 실행시의 상태(idle, pending, succeed, failed)를 각 state의 status로 관리하였으나, 이를 UI에 반영하지는 못하였습니다. 지하철 노선도 미션시에 status 와 useEffect, useSelector 가 섞여서 react rendering flow를 좇아가기가 너무 어려웠습니다.. 이에 loading을 나타내는 spinner 표시는 이번 미션에서는 배제하였습니다...
  • 반응형 레이아웃을 적용하였습니다. breakpoint 는 아래와 같이 나누었습니다.
    • SMALL: "640px",
    • MEDIUM: "768px",
    • LARGE: "1024px",
      제 휴대폰(iPhone XR) 에서는 원하는 대로 잘 표시가 되는데, 다른 폰에서는 어떨지 모르겠습니다...
      CSS cross-browsing 이 생각보다 쉽지 않았습니다.. cross-browsing 의 어려움에 대해 살짝 맛본듯한 느낌입니다 ㅎㅎ

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.

장바구니에 있는 물품을 구매하고나면 장바구니가 비워져야할것같아요! 👍

코드가 전체적으로 깔끔하고 일관성있어서 좋네요! 👍

고생하셨어요! 👏 👏 👏

src/App.jsx Outdated
Comment on lines 22 to 23
dispatch(fetchCart());
dispatch(fetchOrders());
Copy link

Choose a reason for hiding this comment

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

메인페이지에서는 주문목록이 보이지 않는데 모든 정보를 다 가져오면 비효율적이지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

메인페이지에서 물품 정보뿐만이 아니라 장바구니 정보가 필요하여서 물품 정보와 장바구니 정보를 fetch 하여야 하는데,
그냥 주문목록 정보도 같이 fetch 해버릴까... 라는 생각으로 가볍게 접근하였습니다..

말씀해주신 바와 같이 주문목록 정보는 주문목록 페이지로 이동하지 않는 이상 불필요하므로, 주문목록 페이지(src/pages/Order/Order.jsx)에서 fetch 하는 것으로 수정하였습니다! 😄

관련 커밋입니다: 8d7f8c9

src/App.jsx Outdated

return (
<S.App>
<BrowserRouter basename={process.env.PUBLIC_URL}>
Copy link

Choose a reason for hiding this comment

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

환경변수는 const PUBLIC_URL = process.env.PUBLIC_URL 처럼 상수로 따로 분리해서 관리하면 좋습니다! 👍

Copy link
Author

Choose a reason for hiding this comment

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

process.env, 환경변수 자체가 상수인데, 별도로 상수화를 해야할까? 를 잠시 고민했습니다.

고민 끝에 다음과 같은 이유로, src/constants/env.js 를 만들어 export const { PUBLIC_URL } = process.env; 와 같이 상수화하였습니다!

  1. 상수를 사용하는 파일 입장에서는 해당 상수가 환경변수인지 JS파일의 상수인지를 모르게 하는게 좋겠다.
  2. 환경변수에서 파생된 상수를 환경변수와 같이 export 시킬 수 있다.
const NODE_ENV = process.env.NODE_ENV;

const PUBLIC_URL = NODE_ENV === "development" ? "/" : process.env.PUBLIC_URL;

export {NODE_ENV, PUBLIC_URL};

Copy link
Author

Choose a reason for hiding this comment

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

관련 커밋입니다: fc971af

Comment on lines 14 to 44
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
<g>
</g>
Copy link

Choose a reason for hiding this comment

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

오잉 이건 뭘까요 ㅋㅋ

Copy link
Author

Choose a reason for hiding this comment

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

앗 불필요한 <g> 가 들어가 있었네요.. free vector 를 제공하는 사이트에서 다운받아서 아무 생각없이 사용했습니다 😿

MDN에 따르면 svg의 <g>는 HTML의 <div>와 같이 컨테이너 역할을 수행하나, 해당 svg 파일에서는 불필요하다가 판단되어 삭제하였습니다!

짚어주셔서 감사합니다~!

참고자료)
https://developer.mozilla.org/en-US/docs/Web/SVG/Element/g

Copy link
Author

Choose a reason for hiding this comment

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

관련 커밋입니다: fdbaa82

@bigsaigon333
Copy link
Author

장바구니에 있는 물품을 구매하고나면 장바구니가 비워져야할것같아요!

앗 당연히 확인을 하고 리뷰 요청을 드렸어야 했는데, 부끄럽습니다 😿

해당 부분 수정하였습니다~!

관련 커밋입니다: caf02df

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.

피드백 잘 반영해주셔서 감사해요! 👍

고생하셨어요!!! 👏 👏 👏 👏 👏 👏 👏

@ysm-dev ysm-dev merged commit 3ca553e into woowacourse:bigsaigon333 Jun 4, 2021
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

2 participants