-
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
[2단계 - 장바구니 미션] 하루(김하루) 미션 제출합니다. #54
Conversation
학습로그테스트태그
|
학습로그리덕스태그
|
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.
안녕하세요 하루
Vallista님 대신 리뷰하게 되었네요!
아직 고민중인 1단계 피드백
주문 내역 페이지 배경 색상 변경하기
좋습니다!!
rgba(${Color.GRAY_100}, 0.5)와 같이 사용하기
HEXA 코드가 아직 친숙하지 않을 수 있어 그럴 수 있다고 생각합니다!
svg에서 width에 따라 viewBox 수정하기
SVG 다루는 것이 어렵죠?!
차근차근 계속 시도 해봅시다
확장성이 넓어서 위험한 …rest 고민해보기
아쉽게도 Vallista님은 아닙니다만..!
제 개인적인 견해로는 확장성을 뛰어넘는다는 생각이 듭니다.
...rest
를 적극 활용하는 크루들이 간혹 있는데요.
라이브러리를 만드는 관점에서는 매우 유연하고 확장가능하기에 훌륭하지만
서비스를 생산하는 입장에서는 많은 케이스가 나올 수 있기에 위험할 수 있다고 생각합니다!
전반적인 피드백
1.
아직도 <S.Name>
이런 컴포넌트 네이밍을 선호하시는 것 같은데 약간의 아쉬움이 듭니다.
명확한 기준을 두고 분리하는 것 같지만 발목을 잡는 느낌이 든다고나 할까요?
2.
Redux
=> SWR
도입 수준이 아니라 코드를 갈아엎으셨네요!
커스텀 훅도 아주 적절하게 잘 사용해주셨습니다
하지만 아쉬운 부분은 SWR의 장점만 Redux의 단점만 생각하신게 아닐까 싶습니다.
저 같으면 GET
하는 부분의 스토어 코드만 리팩터링 했을 것 같긴한데요
이러한 대규모 리팩터링이 프로젝트에 큰 위험을 가져다주는데 개인적으로 하루에게 많은 학습이 되셨을 것이라는 생각이 듭니다.
다만 전적으로 SWR을 수용하고 완전히 Redux를 제거했다는 느낌이 들 정도의 코드 변경이더라구요
그에 대한 합당한 리스크와 배움 그리고 장점과 단점도 꼭 점검하시길 바랍니다.
3.
데이터 페칭시 반복되는 코드들이 있는데요.
굳이 제가 언급하지 않아도 하루가 스스로 아쉬움이 남았을 것이라는 생각이 듭니다.
if (response.status !== 200) {
throw new Error(response);
}
const body = await response.json();
return deepCamelize(body);
이러한 반복을 어떻게하면 줄일 수 있을까요?!
아무래도 SWR
에 의존적인 부분이다보니 SWR
로 해결하거나 다른 방법이 있을 텐데
향후 좋은 방법을 잘 찾아보시길 바랍니다!
4.
유틸쪽에 굉장히 많은 코드들이 있는데요!
이 모든 것을 직접 작성하신 것이라면
다른 협업 개발자들이 잘 활용할 수 있도록 인자들만이라도 JSDoc
을 활용해서 명시해해주시는게 어떨까요?!
그게 아니라면 JSDoc
을 활용해서 참조했던 링크라도 달아주는게 좋답니다!
다양한 스터디와 활동을 하시면서 만만치 않은 미션과 일정을 소화하시느라 고생이 많으셨네요
그 와중에도 꼼꼼한 PR을 보면 리뷰어 입장에서도 적극 개선 의지가 느껴서 좋습니다.
이러한 습관을 지치지 않는 선에서 잘 유지하시면 더 성장하실 수 있을거라 믿습니다.
이외에 작은 부분들은 코멘트를 남겨놨습니다
고생많으셨어요~
@@ -0,0 +1 @@ | |||
/* /index.html 200 |
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.
🤭
@@ -13,11 +14,13 @@ export const Container = styled.label` | |||
-moz-user-select: none; | |||
-ms-user-select: none; | |||
user-select: none; | |||
color: #333333; | |||
color: ${COLOR.HEX.GRAY_800}; |
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 Container = styled.div` | ||
padding-top: 5rem; | ||
padding-top: ${LAYOUT.NAVBAR_HEIGHT}; |
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.
👍
@@ -0,0 +1,34 @@ | |||
import { Component } from 'react'; | |||
|
|||
import { BlueScreen } from '../../'; |
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.
'../../';
이러한 import path를 선호하시는 이유가 있으신가요~?
<S.StepperButton onClick={onIncrement} children={<UpwardIcon />} isUpward /> | ||
<S.StepperButton onClick={onDecrement} children={<DownwardIcon />} /> | ||
<S.StepperButton onClick={onIncrement} children={<IconUpward />} isUpward /> | ||
<S.StepperButton onClick={onDecrement} children={<IconDownward />} /> |
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.
<S.StepperButton onClick={onDecrement} children={<IconDownward />} />
// or
<S.StepperButton onClick={onDecrement}>
<IconDownward />
</S.StepperButton>
이러한 예시가 나올 수 있을텐데 children
을 따로 명시해서 내려주시는 이유가 있으실까요?
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.
처음에 두 번째로 써주신 것처럼 썼다가 굳이 3줄에 걸쳐서 볼 내용일까?
싶어서 한 줄로 줄여쓰려고 children에 직접 넣어주는 구문으로 변경해보았습니다...! 다시 보니 다른 분이 보시면 왜 이렇게 썼을까 하는 의문이 들 수 있는 코드 모양새 인 것 같아서 앞으로 요론 부분 작성할 때 유의해 보겠습니다...!
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.
리액트에서 Render Props
가 있는데요~
비슷한 방법으로 렌더링하신건가했습니다!
<Router> | ||
<NavBar /> | ||
<Confirm isOpen={isOpen} message={message} onCancel={onCancel} onApprove={onApprove} /> | ||
<Switch> |
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.
Switch
라우터라면 매치되지 않았을 경우의 Default
케이스도 고려하시는게 어떨까요?
import { ROUTE } from '../constants'; | ||
import { useConfirm } from '../hooks'; | ||
|
||
export const App = () => { |
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.
- 진입점에 가까운
App
컴포넌트를 페이지 컴포넌트 하위에 종속시킨 이유가 있으실까요? - 파일명은
index.js
인데 컴포넌트 이름과 다르게 가져가신 이유가 있으실까요?
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.
코드 작성할 당시, 페이지들을 몽땅 불러서 넣는 컴포넌트라 pages에 넣어도 될 것이라고 생각했던 것 같습니다... 이제보니 index.js 에 있을 코드가 아닌 것 같습니다...!
pages/index.js
에서는 각 페이지 컴포넌트들을 export만 시켜주고 상위의 App.js
를 새로 만들고 각 페이지 컴포넌트를 가져와서 (파일명과 같은) App
컴포넌트를 작성하는 것이 더 좋았겠다는 생각이 드네요...! 좋은 피드백 감사드립니다. 🙏
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.
나중에 보고 느끼시는게 있다면 좋습니다~
다음에 다른 프로젝트 진행하시면서 그에 맞는 구조를 또 시도해보세요!
width: 95%; | ||
`; | ||
|
||
export const OrderList = styled(List)``; |
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 OrderList = styled(List)``; | ||
|
||
export const OrderItem = styled.li``; |
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.
볼때마다 아쉬움이 남는 부분인데요
나중에는 방법으로 표현할 수 있을지 고민해보는 것을 추천합니다!
const { product, isLoading, isError } = useProduct(productId); | ||
const { addProduct } = useCart(); | ||
const handleImageError = (e) => { | ||
// TODO: 이미지 로딩 실패 시 에러 처리 |
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.
기대하겠습니다~
발리스타님, 안녕하세요!
지난 번에 인사드렸던 🙋🏻♀️ 하루(@365kim) 입니다~! 🇮🇳🥘
지난 5월 12일에 1단계를 제출하고, 6월 11일에서야 2단계를 제출드리네요... 무려 한 달 만에..😭😭😭
기다려주셔서 정말 감사드립니다...! 🙏
2단계 구현 과 1단계 피드백 주셨던 부분도 함께 PR 올려봅니다!
✅ 2단계 구현기능 목록 - 데모 페이지
API 연결
상품목록
상품상세
💪 새롭게 시도한 부분 - SWR 적용
1단계 피드백에서 리뷰어님께서 추천해주신 data fetching을 도와주는 SWR 라이브러리를 도입해보았습니다.
원래 리덕스에서 들고있던 상태가 대부분 fetching해온 데이터 이다보니, 관련된 action, action creator, reducer 몽땅 날릴 수(?) 있었습니다. 😲 DB랑 리덕스 상태 싱크 맞추는 고민도 안해도 되어서 편-안하게 느껴졌습니다. 😲 또 isLoading, isError를 너무 쉽게 뽑아올 수 있어서 상품목록의 스켈레톤 UI를 입히기도 수월했던 것 같습니다 😲
SWR에서 현재 코드 상에는 사용한 기본적인 구문 외에도 다양한 옵션을 제공해서 더 잘 사용하면 편리함+유용함이 가득한 도구인 것 같습니다! ㅎㅎ
캐싱이 될 것 같았는데 네트워크 요청이 발생하는 경우가 있어, 정확히 언제 캐싱이 되고 캐싱 주기를 어떻게 컨트롤 할 수 있는지 사용해보면서 좀 더 익혀보려고 합니다 💪💪💪
📌 1단계 리뷰반영 요약표
👉추천해주신 SWR를 적용해보았습니다.
4b0c093 c8fbf50
👉에러바운더리 컴포넌트, 스켈레톤 컴포넌트를 구현해보았습니다.
👉 pages 디렉토리 src 바로 하위로 분리했습니다.
👉 useCart, useProducts, useOrder, useProduct 추가해보았습니다.
c8fbf50 47c35a5
👉 redux와 결합을 낮추기 위해 Confirm에 의존성 주입했습니다.
👉 COLOR, BREAK_POINT 등 상수로 관리했습니다.
👉 크기가 0인 input의 위치를 화면 밖으로 조정했습니다.
👉 calc(100% - STEPPER_WIDTH)로 변경했습니다.
👉 내용물이 꼭 필요한 경우 PropTypes를 isRequired로 설정,
아닌 경우 조건부 렌더링 처리했습니다.
👉mock.js 로 분리했습니다.
👉 테스트 관련 패키지 devDependencies 로 이동시켰습니다.
🙋🏻♀️ 아직 고민중인 1단계 피드백
…rest
고민해보기 : 바로가기크고 작은 개선사항 코멘트로 전해주시면 열심히 개선해보겠습니다! 💪💪💪
잘 부탁드리겠습니다! 🙇🏻♀️🙇🏻♀️🙇🏻♀️
감사합니다...! 🌷