-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[장바구니 미션 Step 1] 황펭(양이진) 미션 제출합니다. #173
Conversation
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
Co-authored-by: sᴏʟʙɪ ☔️ <solbi2004@naver.com>
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.
안녕하세요 황펭! 이번 미션의 리뷰어를 맡게 된 체프라고 합니다. 만나서 반가워요! 😃
코드를 쭉 살펴봤는데 전체적으로 깔끔해서 딱히 드릴 만한 피드백이 없었어요. 상태 관리 라이브러리의 사용도 깔끔했고요.
남겨드린 피드백은 꼭 반영하실 필요 없습니다. 질문 드린 부분에만 답변해주시면 빠르게 다음 단계로 넘어가실 수 있도록 바로 어프루브하겠습니다. 수고하셨습니다!
이번에 진행한 방식은 스토리북을 이용하니 컴포넌트 ui를 바로 확인하고 스타일을 수정해보고 선 진행 후 페이지를 만드니 생각보다 조립이 쉽더라구요 👍
네! 그와 동시에 프로젝트에서 사용되는 컴포넌트를 바로 리스팅할 수 있다는 장점도 있다고 생각해요. 각각 장단점이 있지만 개인적으로는 컴포넌트를 먼저 만들고 조립해나가는 방식을 선호한답니다 😄
package.json
Outdated
"@types/react-dom": "^18.2.4", | ||
"@types/styled-components": "^5.1.26", | ||
"babel-plugin-named-exports-order": "^0.0.2", | ||
"prop-types": "^15.8.1", |
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.
타입스크립트가 있는데 prop-types를 설치하신 이유가 있으셨을까요?
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.
storybook init을 이용해 설치하다보니 불필요한 패키지가 있었네요 😅
불필요한 패키지 삭제했습니다.!
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.
아하, 의도한 부분은 아니셨군요. 답변 감사합니다 :)
src/GlobalStyle.tsx
Outdated
:root { | ||
--primary-color:#333333; | ||
--background-color: #F5F5F5; | ||
--alert-color: #04C09E; | ||
|
||
--gray-100:#DDD; | ||
--gray-200: #CCC; | ||
--gray-300: #BBB; | ||
--gray-400: #AAA; | ||
} |
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.
CSS global variables도 좋지만, styled-components의 theme color나 자바스크립트를 이용하여 상수화하는 게 더 좋지 않나 생각해요. CartIcon 컴포넌트의 color 값을 넘겨줄 때, CSS의 문법을 지켜주어야 하는 불편함을 개선할 수 있지 않을까요?
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.
colors 객체를 만들고 ThemeProvider 이용해 테마를 전달해주는 방법으로 수정했습니다.!
추가로 globalstyle의 css 코드를 분리하고 styles 폴더를 두었습니다.
자바스크립트 객체로 접근하니 색상 키값을 전달해주는 편안함이 있네요 😄
src/hooks/useCartProducts.ts
Outdated
const findTargetProduct = (cartProducts: CartProduct[], id: number) => | ||
cartProducts.find((cartProduct) => id === cartProduct.product.id); | ||
|
||
const deleteProduct = (cartProducts: CartProduct[], id: number) => | ||
cartProducts.filter((cartProduct) => cartProduct.product.id !== id); |
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.
각 함수가 어떤 역할을 하는지 네이밍을 부여하셨군요! 굉장히 좋은 코드라고 생각합니다 :)
src/hooks/useCartProductUpdate.ts
Outdated
import { cartProductState } from '../states/cartProductState'; | ||
|
||
const useCartProductUpdate = () => { | ||
const [cartProducts, setCartProducts] = useRecoilState(cartProductState); |
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.
useRecoilState를 컴포넌트에서 바로 사용하지 않고 로직을 커스텀 훅으로 묶으셨군요 👍
src/hooks/useCartProductStorage.ts
Outdated
type SetStoredCartProducts = (cartProducts: CartProduct[]) => void; | ||
type UseCartProductStorage = [StoredCartProducts, SetStoredCartProducts]; | ||
|
||
const STORAGE_ID = 'shop-cart'; |
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.
localStorage id의 상수화까지! 👍
src/components/Common/Header.tsx
Outdated
import { totalCartProductState } from '../../states/cartProductState'; | ||
|
||
const Header = () => { | ||
const totalCartProduct = useRecoilValue(totalCartProductState); |
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.
(미세먼지만큼 작은 피드백) 앞서 드린 리뷰와 비슷한 맥락으로 useCartCount와 같은 커스텀 훅으로 묶어서 사용해도 저는 나쁘지 않다고 생각해요. 코드에 네이밍을 부여하는 것은 좋은 코드라고 생각하거든요.
<dl> | ||
<ProductName>{name}</ProductName> | ||
<ProductPrice>{price.toLocaleString('ko-KR')} 원</ProductPrice> | ||
</dl> |
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.
시맨틱한 태그 사용 너무 좋습니다 👍
useEffect(() => { | ||
const getProducts = async () => { | ||
const data = await fetchProducts(); | ||
setProducts(data); | ||
}; | ||
|
||
getProducts(); | ||
}, []); | ||
|
||
useCartProductUpdate(); |
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.
useProducts라는 커스텀 훅으로 묶을 수 있을 것 같지 않나요?
docs/REQUIREMENTS.md
Outdated
- [ ] 적합한 테스트 도구를 선택하여 사용하고, 중요한 테스트 케이스를 정의하여 테스트 진행 | ||
- [ ] 스토리북 테스트 작성 |
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.
테스트 코드를 작성해야 하는 것으로 알고 있는데, 이번 단계에서는 포함하지 않으신 이유가 있으셨을까요?
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.
테스트 코드 작성까지 시간이 부족해 미처 진행하지 못했습니다..🥲
ProductItem과 페이지 스토리에서 interaction 작성해보았습니다.!
체프 리뷰 감사드려요 😊 주말 잘 보내고 계신가요??
AtomEffect로컬스토리지 코드를 atomEffect에서 동작하게 수정했습니다. 그러다 atomEffectf라는 개념을 발견했는데요, 리액트 컨텍스트 외부에서 부수효과를 관리하더라고요..! 사실 cartProductState의 value는 로컬스토리지 업데이트 외에는 사용하지 않았기에, 관련 로직을 옮기면서 관련 커스텀 훅을 없앨 수 있었고, ProductList를 구독하지 않는 형태가 되었습니다! 장바구니 품목의 상태나 부수효과를 atom에서 모두 정의할 수 있었고, 렌더링 최적화도 할 수 있었습니다 😊 atom 외에 다른 기능은 사용해 보지 않았는데 많은 기능들이 있었네요..! |
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.
리코일에 대해서 추가로 학습을 진행하고 리팩토링까지 진행해주셨군요! 👍
사실 이번 미션은 상태 관리 흐름을 위주로 리뷰를 진행했기 때문에 리코일 자체에 대한 리뷰까지는 많이 진행하지 않았어요. 제가 리코일에 대한 사용이 미숙한 점도 있었구요 😓
황펭 덕분에 오히려 atomEffect을 이용해서 리렌더링을 최적화할 수 있는 사례에 대해서 알게 되었습니다. 감사합니다 😃
리코일에 대해서 학습을 추가로 진행하셨고, 직접 적용도 해보셨으니 질문을 한 가지 드려보고 싶어요.
- 상태 관리 라이브러리로서의 리코일의 장점과 단점에 대해서 어떻게 생각하시나요?
- 어떤 경우에 리코일을 적용하는 것이 좋을까요?
다음 단계도 화이팅입니다 💪
atom 외에 다른 기능은 사용해 보지 않았는데 많은 기능들이 있었네요..!
상태 관리가 주요 고민이지만, 이왕 recoil을 쓰니까 좀 더 기능을 활용해 보고 싶은 마음이 듭니다.
하지만 아직 익숙하지 않고 unstable 한 부분도 있어서 어디까지 recoil의 기능을 활용해야 할지 고민이 생깁니다..
이런 상황(라이브러리를 학습하는 과정)에서 체프는 어떻게 학습하시나요.?
우선 제가 라이브러리를 학습하는 과정을 설명드리는 게 좋을 것 같네요.
- 공식 문서와 함께 블로그 글을 함께 찾아봐요. 블로그 글을 같이 찾아보는 이유는 좀 더 쉬운 언어로 작성되어 있다고 생각하거든요. 이해를 바탕으로 풀어낸 글은 훨씬 이해하기 쉽다고 생각해요. 이후에 한 번 더 공식 문서를 읽어보는 편입니다. (다만... 개인적으로 리코일은 공식 문서 번역에 조금 어색한 점이 많다고 느껴서요... 학습이 조금 어렵다고 생각되네요 😅)
- 예시 코드를 직접 작성해봅니다. 글로만 읽는 것보다 직접 코드로 치는 과정이 꼭 필요하다고 생각하거든요.
- 처음부터 모든 기능을 싹 훑어보면서 공부를 진행하지는 않아요. 주요 개념에 대해서 공부하고 적용해보고, 다른 기능이 있는 것을 알게 되면 그 기능을 공부해보고, 적용할 필요가 있으면 수정 적용해보고. 그런 느낌으로 공부하고 있어요.
- 정리하자면 요런 느낌이겠네요.
- 공식 문서 가볍게 훑어보고 -> 블로그 글들을 탐방하고 -> 직접 코드를 작성해보고 -> 공식 문서 다시 읽기
- 주요 기능부터 학습하고 코드를 작성해본다 -> 여유가 있을 때, 혹은 주요 기능의 한계점을 알게 되었을 때 새로운 기능에 대해서 공부한다 -> 적용한다
unstable한 기능에 대해서는 충분히 인지하고 학습을 진행해야 한다고 생각해요. unstable하다고 공식 문서에 적혀있으면, 사이드 프로젝트나 학습 과정에서는 사용할 수 있지만, 절대 회사에서는 사용하지 않아야 한다고 생각해요. 만약에 중요한 기능이 unstable하면, 다른 라이브러리를 찾아봅니다 😓
이번에 황펭이 리코일을 리팩토링 진행하시면서, 좋은 기능을 적재적소에 잘 넣어주셨다는 인상을 많이 받았어요. 그래서 이번 경우에는 크게 걱정하지 않으셔도 좋을 것 같아요. 😃
추가로 recoil을 사용해 보셨다면, 사용해 볼 만한 다른 기능이 있을까요?
상태 변경의 추이를 살펴볼 수 있는 스냅샷 기능에 대해서도 공부해보시면 어떨까 싶네요! 디버깅에 굉장히 유용한 기능 중 하나랍니다. 저는 상태 변경을 추적할 수 있다는 점이 Redux의 가장 큰 장점이라고 생각하는데요. 스냅샷을 이용해서 Recoil에서도 이와 비슷한 기능을 구현할 수 있습니다.
https://recoiljs.org/ko/docs/api-reference/core/Snapshot
https://taegon.kim/archives/10126
export const localStorageEffect = | ||
<T>(key: string): AtomEffect<T> => | ||
({ trigger, setSelf, onSet }) => { | ||
if (trigger === 'get') { | ||
setSelf(JSON.parse(localStorage.getItem(key) ?? '[]')); | ||
} | ||
|
||
onSet((newValue) => { | ||
localStorage.setItem(key, JSON.stringify(newValue)); | ||
}); | ||
}; |
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 targetCartProductState = selectorFamily({ | ||
key: 'targetCartProductState', | ||
get: | ||
(id: number) => | ||
({ get }) => | ||
findTargetProduct(get(cartProductState), id), | ||
}); |
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.
selectorFamily도 활용하셨군요 👍
배포주소
스토리북
안녕하세요 체프! 장바구니 미션 리뷰 요청 드립니다 😊
이번 미션은 해온과 페어로 진행했습니다.
요구 사항
컴포넌트 구조
진행 과정
기존에 제가 미션을 하면서 페이지를 먼저 만들고 컴포넌트로 분리해 나갔습니다. 이번 미션은 컴포넌트를 먼저 만들고 스토리북으로 확인하면서 조립해 페이지를 만들어 보았습니다.
확실히 기존에 진행하던 방식은 전체적인 그림을 보기 위해서 했어서, 하나하나의 컴포넌트 스타일을 관리하기 어려운 부분이 있었습니다.
이번에 진행한 방식은 스토리북을 이용하니 컴포넌트 ui를 바로 확인하고 스타일을 수정해보고 선 진행 후 페이지를 만드니 생각보다 조립이 쉽더라구요 👍 스토리북의 더욱 알 수 있었습니다. (인터렉션은 후 진행해보려 합니다..😅)