-
Notifications
You must be signed in to change notification settings - Fork 163
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단계 - 장바구니 미션] 하루(김하루) 미션 제출합니다. #5
Conversation
Co-authored-by: 0imbean0 <tjsqls2003@gmail.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <tjsqls2003@gmail.com>
Co-authored-by: 0imbean0 <tjsqls2003@gmail.com>
Co-authored-by: 0imbean0 <tjsqls2003@gmail.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
Co-authored-by: 0imbean0 <0imbean0@users.noreply.github.com>
/* atoms */ | ||
export { Button } from './atoms/Button'; | ||
export { CartIcon } from './atoms/CartIcon'; | ||
export { Checkbox } from './atoms/Checkbox'; | ||
export { DownwardIcon } from './atoms/DownwardIcon'; | ||
export { Line } from './atoms/Line'; | ||
export { List } from './atoms/List'; | ||
export { Template } from './atoms/Template'; | ||
export { TrashCanIcon } from './atoms/TrashCanIcon'; | ||
export { UnderlinedText } from './atoms/UnderlinedText'; | ||
export { UpwardIcon } from './atoms/UpwardIcon'; | ||
|
||
/* molecules */ | ||
export { CheckoutBox } from './molecules/CheckoutBox'; | ||
export { Confirm } from './molecules/Confirm'; | ||
export { Header } from './molecules/Header'; | ||
export { NavBar } from './molecules/NavBar'; | ||
export { QuantityStepper } from './molecules/QuantityStepper'; |
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.
질문드립니다! 🙋🏻♀️ - 컴포넌트 위계 구성방법
atom으로는 아무것도 모르는 컴포넌트를, molecule로는 필요 시 도메인을 알거나 로직을 가질 수 있도록 활용했습니다.
이전 미션에서는 재사용 가능한 컴포넌트를 위계 구분 없이./commons
에 넣어서 작성했었는데,
아주 심플한 컴포넌트와 꽤 복잡한 컴포넌트가 위계 구분 없이 ./commons
에 몽땅 들어있을 때보다 코드를 관리하기 편하다고 느껴졌습니다.
이런 방식으로 컴포넌트를 구성하는 것이 괜찮은 방법일지 궁금합니다...! 👉👈
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.
저희 팀도 과거에는 아토믹 컴포넌트를 이러한 형태로 사용하였습니다! 지금은 디자인 시스템으로 통합해서 사용하지 않지만요!
이러한 아토믹 컴포넌트를 사용하면서 Organisms 및 Template 레벨은 프로젝트의 한정된 레이아웃을 뽑는데 좋았고, atoms과 molecules로 공통된 레이아웃 컴포넌트를 만드는데 좋았습니다! 그래서 Organisms 레벨부터는 비즈니스 로직를 보관하는 container와 1:1 로 매칭시켜서 비즈니스 로직을 녹여내는데에도 좋았던 기억이 있네요.
사용하신 방법처럼 요렇게 공통화를 해서 진행하는게 나쁘지 않다고 생각이 들어요~
다만, 지금과 같이 한 파일에서 한데 모아 import와 export를 진행하는게 webpack 빌드상 일반적으로 import를 하는 방식과 어떻게 다를지는 고민해보면 좋겠습니다!
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.
컴포넌트 분리의 미학 요건 제가 과거에 썻던 글인데, 한 번 읽어보시면 다양한 관점으로 접근하실 수 있을 것 같아 공유드립니다 👍
export const INITIAL_STATE = {}; | ||
|
||
export const INITIAL_CART_PRODUCT_PROPS = { | ||
quantity: 1, | ||
isSelected: true, | ||
}; | ||
|
||
export const cartReducer = (state = INITIAL_STATE, action) => { | ||
const { type = '', payload = '' } = action; | ||
|
||
switch (type) { | ||
/* payload: product */ | ||
case ADD_PRODUCT: | ||
return { ...state, [payload.id]: { ...payload, ...INITIAL_CART_PRODUCT_PROPS } }; | ||
|
||
/* payload: id */ | ||
case REMOVE_PRODUCT: | ||
return getPropertyRemoved({ ...state }, payload); | ||
|
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.
질문드립니다! 🙋🏻♀️ - state 가공 위치
서버로부터 받아온 product라는 객체에 상태관리에 필요한 property 몇 개를 더 추가해서 redux에 상태로 저장하려는 상황이 있었습니다. 이 때 프로퍼티를 추가해서 필요한 state를 만드는 작업은 어디서 하는 것이 맞을지 질문 드립니다..!
1. dispatch하는 주체가 할 경우, ⇒ 다른 곳에서도 동일한 액션을 dispatch할 때 계속해서 가공해줘야하는 문제가 있습니다. 🙅🏻♀️
2. action creator가 할 경우 ⇒ 이름을 보면 전달자 역할만 할 것 같아서 역할에 맞지 않는 듯 합니다. 🙅🏻♀️
3. reducer가 할 경우 ⇒ 이전 state를 받아 새로운 state를 반환하는 역할에 맞다고 판단해서 여기에 넣었습니다. 🙆🏻♀️
그런데 막상 작성하고나니 reducer 내 에서 각 action서 전달받는 payload의 내용이 무엇인지 파악하기 힘들었습니다. 코드를 처음보는 사람이 각 reducer 내의 payload가 어떻게 생겼는지 알려면 결국 dispatch하는 곳에서 무엇을 전달하고 있는지 봐야했습니다.
이런 불편함을 줄이고자 switch문 내에서 각각의 payload가 어떻게 생겼는지 주석을 달아두게 되었습니다. 주석을 달고 나니 혹시 이게 불필요한 주석은 아닐지, 더 좋은 작성방법이 있는게 아닐지 궁금해졌습니다...! 지금처럼 작성하는 방법이 괜찮은지, 혹시 더 좋은 방법이 있을지 조언해주시면 감사하겠습니다...!! 🙏
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.
저는 state만드는 작업은 reducer에서 해주어야 한다고 생각합니다. 그리고 state payload에 대한 문제는 typescript를 적용하면 해결이 되지 않을까 싶어요~
주석으로 넣는 방법도 좋지만 getPropertyRemoved function에서 명세를 해주어도 좋겠네요~
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.
상태 관리 라이브러리 = redux니, 상태 관리는 redux에서 해주는게 맞겠죠!
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.
안녕하세요 하루!
장바구니 미션 수고 많으셨습니다! 코드량이 많아서 코드리뷰가 오래 걸렸네요 ㅠㅠ
UX 피드백
-
주문내역의 백그라운드 색이 다른 페이지와 다릅니다 ㅠㅠ 그래서 살짝 눈아픈데 변경을 해주셨으면 좋겠어요!
-
장바구니나 주문내역등 다른 페이지에서 다시 샵으로 복귀할 때, 캐싱을 사용해서 짧은 시간이라면 api fetching을 하지 않도록 제공하는 것도 좋겠습니다. 짧은시간동안 상품 목록이 갑자기 전면 개편될 일은 없을 것이고, 대다수의 경우에는 (타임 세일이나 그런 부분이 없다면) API Call을 줄일 수 있는 방법이기 때문입니다. 더불어, API 요소가 달라진 부분이 없다면 갱신을 안시키는 로직도 들어가있으면 좋겠네요. 사용자 UX 상으로는 백그라운드에서 동기화가 되니, 상관없이 잘 이용할 수 있게 제공도 해주어야 겠지만요. swr 같은것도 써보면 좋겠습니다.
-
로딩을 만드는게 좋겠습니다~ API 로드가 길어지면 아무런 화면도 띄워지지 않아, 사용자 입장에서 버그라고 생각할 수 있을 것 같아요~ 또한 타임아웃때도 어떤 에러페이지가 나올 지 생각해보면 좋겠습니다!
전반적인 코드 피드백
-
개인적으로 Components안에 Page가 있어야 하는게 맞는지 의문스럽습니다. Page는 Component일까요? Page와 Component 레벨을 분리하여 페이지에서 컴포넌트를 가져다가 레이아웃을 만든다는 흐름이 더 적절해보이며, 페이지는 컴포넌트로써 기능을 하고 있는걸로 보이지 않습니다!
-
hooks로 땔 수 있는 로직이 굉장히 많아보입니다~ redux 상태와 레이아웃에 들어가는 비즈니스 로직이 엮여있는 부분을 분리해낼 수 있어 보이구요~ 비단 공통된 로직만 분리한다는 개념보다는 비즈니스 로직을 분리한다는 개념으로도 사용해도 좋겠습니다~ 컴포넌트의 재사용화를 위해서요!
-
지금은 양이 적어서 ducks 패턴을 사용하면서 파일 하나에 명세가 되지만, 추후에는 분리를 해도 좋겠습니다. 제가 쓴 글 에서 중간 스크롤 쯤에 있는 이미지를 참고해주세용
-
나머지는 코드 라인에 상세하게 적어두었으니 함께 보시면 좋겠습니다~
"version": "0.1.0", | ||
"private": true, | ||
"dependencies": { | ||
"@testing-library/jest-dom": "^5.12.0", |
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.
testing-library가 dependencies로 있는 이유가 무엇일까요~?
position: absolute; | ||
opacity: 0; | ||
cursor: pointer; | ||
height: 0; |
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.
top left -9999 로 위치를 이동시키는 것도 좋겠습니다~ 만에하나... 0,0 이 브라우저단위에서 인식이 안되어 1,1 이라도 되면 호호.. 찝힐수도 있기 때문이에여. 그리고 0,0은 화면안에 렌더링이 되는 오브젝트로 인식되기 때문에, frustom culling 측면에서도 화면밖으로 빼주는게 좋습니다.
Accelerated Rendering in Chrome 크롬의 accelerated rendering에 대한 자료입니다. 최근의 브라우저들은 결국 하드웨어 (graphic card)등의 가속의 이점 (모바일 디바이스는 APU겠죠?) 을 쓰기 위해서 이미지로 변환을 한 다음, 이미지에서 하드웨어 가속이 일어나는 부분들에 대해 성능을 올리고 있습니다. 그래서 그러한 레이어 모델 상에서, 어떤 형태로 코딩이 되는질 생각해봐도 좋겠습니다.
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.
frustum culling in opengl 프러스텀 컬링이라고 하는 기법은 전통적인 3D, 2D등의 개념에서 카메라(보이는 뷰) 밖의 물체에 대한 최적화를 하는 기술인데, web에서도 이러한 명칭은 쓰지 않지만, 내부적으로는 사용하고 있을 것입니다.
|
||
export const Line = styled.span` | ||
display: inline-block; | ||
width: ${(props) => props.length}; |
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.
요런걸 바이패스 해주는것이라면, styled-component 보다는 emotion의 css props를 쓰는게 더 좋겠습니다. css props
import * as S from './style.js'; | ||
|
||
export const List = (props) => { | ||
const { children, ...rest } = props; |
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.
...rest인 이유는 뭘까용?
export const Container = styled.div` | ||
padding-top: 5rem; | ||
width: 100vw; | ||
min-height: calc(100vh - 5rem); |
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.
- 5rem
을 해주는 이유가 뭘까용?
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.
box-sizing: border-box가 아니기 때문일까용?
export const INITIAL_STATE = {}; | ||
|
||
export const INITIAL_CART_PRODUCT_PROPS = { | ||
quantity: 1, | ||
isSelected: true, | ||
}; | ||
|
||
export const cartReducer = (state = INITIAL_STATE, action) => { | ||
const { type = '', payload = '' } = action; | ||
|
||
switch (type) { | ||
/* payload: product */ | ||
case ADD_PRODUCT: | ||
return { ...state, [payload.id]: { ...payload, ...INITIAL_CART_PRODUCT_PROPS } }; | ||
|
||
/* payload: id */ | ||
case REMOVE_PRODUCT: | ||
return getPropertyRemoved({ ...state }, payload); | ||
|
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.
상태 관리 라이브러리 = redux니, 상태 관리는 redux에서 해주는게 맞겠죠!
} from './cartReducer'; | ||
|
||
const mockId = '1'; | ||
const mockProduct = { |
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.
mock 파일은 .mock.js 정도로 분리하는게 좋겠습니다~
}); | ||
}); | ||
|
||
it('CHECKOUT 액션을 받을 경우, cartReducer는 주문한 상품을 제외한 state를 반환한다.', () => { |
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.
테스트가 자세하니 좋네요
approve: null, | ||
}; | ||
|
||
export const confirmReducer = (state = INITIAL_STATE, action) => { |
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.
개인적으로 confirm을 reducer로 따로 분리할 필요는 없어보이고, 에러에 대한 reducer면 좋겠네요. confirm을 따로 나눈 이유는 에러나 모달을 관리하기 위함이니,
grid-template-columns: repeat(1, 1fr); | ||
padding: 0; | ||
|
||
@media (min-width: 520px) { |
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.
요런 최소 크기 등은 constant로 관리하면 좋겠습니다~
요 것은 추후에 2~3단계 하면서 고려해보세요! 머지하도록 하겠습니다! |
* chore: js로 react 웹팩 + babel 설정 * chore: Webpack + babel 설정하기 (TS로 설정) * chore: Styled-Component 설치 및 Webpack 설정 * chore: 프로젝트 절대경로 설정 * chore: react-router-dom 설정 * chore: Webpack 이미지, 폰트 로드 설정하기 * chores: eslint + prettier 설정 * chore: husky, lint-staged 설정 완료 * chore: Github Action 설정하기 * chore: recoil 설정 완료 * chore: storybook 설정 완료 [#2] * feat: Header UX/UI 구현 [#4] * feat: Spinner 컴포넌트 구현 [#5] * refactor(app): step1 코드 전체 복사 및 웹팩 마이그레이션 * chore: github 액션 테스트 코드 실행 추가 * chore: msw 셋팅 [#6] * fix(constant): 바뀐 서버 URL 수정 * feat(mocks): msw 임의의 백엔드 로직 구현 * refactor(MultiSelector): Compound Component로 MultiSelector 직접 구현 * refactor(type): Product 타입 수정 및 적용 * feat(img): 휴지통 아이콘 로컬에 저장 * feat(mock): CartHandler 코드 수정 * refactor(global): 공통된 a 태그 스타일 수정 * refactor(constant): URL 전역 상태 수정 * feat(CartItem): CartItem 컴포넌트 구현 * refactor(Header): 헤더 컴포넌트 구현 * feat(hooks): useQuery 커스텀 훅 구현 * feat(CartButton): CartButton 공통 컴포넌트 구현 * feat(CheckBox): CheckBox 공통 컴포넌트 구현 * refactor(useCart): useCart에 mock server 요청 로직 추가 * refactor(ProductList): ProductList 컴포넌트 리팩토링 * refactor(ProductItem): 공통 컴포넌트 CartButton으로 인한 코드 리팩토링 * refactor(atoms): product fetch 코드 수정 * feat(PaymentBox): PaymentBox 컴포넌트 구현 * feat(CartDetailPage): CartDetailPage 컴포넌트 구현 * feat(ProductPage): 기존에 사용하던 Suspense, ErrorBoundary 제거 * feat(fetcher): fetcher 함수 구현 * chore(eslintrc): eslintrc 설정 추가 * chore: 개발 환경 셋팅 수정 * chore: 배포 환경 설정 변경 * chore: 배포 환경 설정 변경 2 * chore: 배포 환경 설정 변경 3 * chore: 배포 개발 환경 셋팅 * chore: 배포 개발 환경 셋팅 4 * chore: 모든 파일 삭제 * fix: pull request 안되는 문제 해결 * refactor: useCart 테스트 코드 리팩토링 * feat: 에러 상황에 따른 에러 코드 보여주기 기능 추가 및 테스트 코드 추가 * refactor(useQuery): catch 구문일 때 error 처리로 코드 변경 * refactor(cartItem) 가독성을 위한 삼항연산자 제거
발리스타님, 안녕하세요!
처음 인사드리는 🙋🏻♀️ 하루(@365kim) 입니다.
저는 심바(@0imbean0)과 함께 페어프로그래밍을 진행했습니다.
크고 작은 개선사항 코멘트로 전해주시면 열심히 개선해보겠습니다! 🙏
잘 부탁드리겠습니다! 🙇🏻♀️🙇🏻♀️🙇🏻♀️
🛒 1단계 데모 페이지
💪 새롭게 시도한 부분
컴포넌트 관리 - 아토믹패턴 변형
재사용 가능한 컴포넌트를 아톰(atom)과 몰러큘(molecule) 두 개로 구분하여 관리하였습니다.
CSS in JS - Styled-Compoenent
페어의 도움을 받아 styled-component를 미션에 처음 도입해보았습니다. 필요한 변수를 import해올 수 있고 컴포넌트 합성도 간편해서 앞으로 계속 사용하고 싶다고 생각했습니다.
상태 관리 - Redux
리덕스를 새롭게 학습하고, react-redux를 활용하여 상태관리를 했습니다.
연관된 action과 reducer를 하나의 파일에서 함께 보는 것이 코드를 관리하기 더 수월할 것이라고 생각해서
~Reducer
라는 이름의 파일에 Action Type, Action Creator, Reducer를 함께 정의해주었습니다.데이터베이스 - Firebase
주문 목록 관리를 위해 firebase를 도입했습니다. 처음 사용해보는데도 약 30분만에 적용을 마칠 수 있었을 만큼 사용방법이 간단해서 매력적이라고 생각했습니다...!
📁 디렉토리 구조
✅ 구현 기능 목록
내비게이션바
상품목록
장바구니
주문/결제