-
Notifications
You must be signed in to change notification settings - Fork 24
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
[함헌규] sprint5 #83
The head ref may contain hidden characters: "react-\uD568\uD5CC\uADDC-sprint5"
[함헌규] sprint5 #83
Conversation
- sprint4에서 사용했던 주소를 sprint5에 맞게 변경 - getProducts에 입력할 파라미터 추가 - articleService 제거 (이후에 사용할 때 다시 구현) - 좋아요를 누르거나 취소하는 api 함수 추가
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을 통해서 컴포넌트들에 대한 관리도 이루어질 수 있는것도 좋고요.
리액트를 사용하면서부터는 코드의 분리와 결합의 싸움이 됩니다. 어떻게하면 잘 나누고, 어떻게하면 유연하게 결합할 수 있는지 고민이 깊어질 수 있습니다.
src/pages/Products.jsx
Outdated
function Products() { | ||
const [screenWidth, setScreenWidth] = useState(window.innerWidth); | ||
useEffect(() => { | ||
window.addEventListener("resize", (e) => setScreenWidth(e.currentTarget.innerWidth)); |
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.
화면의 크기가 변경될때마다 setState를 해주고 계시네요.
최상위에서 state를 다루고계시기때문에 아마 화면크기를 전체적으로 살짝식만 움직여도 화면의 전체가 리렌더링 될것같아요. 이는 성능상에 큰 문제점으로 연결됩니다.
반응형 디자인에서는 css의 repaint와 reflow의 개념에대해서 생각해보시면 좋을것 같아요.
약간의 힌트를 드리자면, 일반적인 css를 사용하는것 처럼(현재는 styled-component를 사용하시나 여기에도 동일하게 존재합니다) 레이아웃이 아에 변하는 상황이라면 MediaQuery를 사용하여 break-point를 지정해두고 그때 레이아웃이 변하도록 하여 reflow 지점을 체크할 수 있습니다. 또한 레이아웃이 아에 변하는게 아니라 padding, margin값등 여백이나 크기가 줄어드는 것들은 최대한 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.
이 부분도 수정했습니다! 새롭게 사용해본 기술이라 굉장히 유익한 경험이었습니다. 감사합니다!
try { | ||
setLoading(true); | ||
setError(null); | ||
const { list } = await getProductList({ |
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.
sideEffect가 발생할 수 있는 부분을 useEffect를 통해서 제어해주신점은 좋습니다.
두번째로 생각해봐야 할 것은 getProductList
의 재사용성에 대하여 고민해볼 수 있을것 같아요.
해당 API를 사용할때 필요한정보가 response(data), error, loading 값등이 있을 수 있습니다.
때문에 customHook 형식으로 해당 API호출부를 모듈화해서 관리할 수도 있을것 같아요.
힌트는 productList를 호출하는 부분을 이렇게 모듈화하여 재사용 할 수 있다는 점 입니다.
const { loading, error, products, fetchProducts } = useFetchProductList();
useEffect(() => {
fetchProducts({ page, pageSize, searchKeyword, productSorting });
}, [fetchProducts, page, pageSize, searchKeyword, productSorting]);
</ProductButtonsContainer> | ||
</ProductsBar> | ||
<ProductsContainerComponent> | ||
{/* 로딩,에러 핸들 구현 예정 */} |
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/components/products/Product.jsx
Outdated
} | ||
`; | ||
|
||
function Product({ title, price, likes, image, best = false }) { |
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.
trdl;
컴포넌트를 공통화하려고 추상화하려면 내부에 도메인적인 사항이 없을수록 재사용성이 높아집니다.
상품을 Best, Normal 두가지를 재사용하기위하여 컴포넌트를 만들어주신점 좋습니다!
다만 컴포넌트를 사용할땐 best
라는 값이 분류의 기준이 될 수 있지만 이렇게 재사용되는 컴포넌트는 특정 도메인(여기서는 상품의 종류) 에 대하여 모르고 있어야 합니다.
이것이 바로 컴포넌트의 재사용성을 높히기 위한 추상화에 해당하는 개념인데요, 해당 컴포넌트는 본인이 best상품을 렌더링할지, normal상품을 렌더링할지를 모르는게 좋습니다. 여기서분류할 수 있다면 크기 를 기준으로는 분리할 수 있을것같아요.
실제로 경험하게될 비즈니스의 세계에서는 언제 어떤 컴포넌트가 어떻게 수정될지는 예측하기 어렵습니다. 때문에 어떤 도메인의 어떤값 등으로 분류하는것 보다 좀더 추상화된(목적)으로 컴포넌트를 분류해주시는게 좋습니다.
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.
이부분도 반영해서 수정했습니다 좋은 내용 감사합니다!
|
||
return ( | ||
<> | ||
<BestProductSection> |
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.
어떻게 보면 BestProductContainer
과 ProductContainer
는 동일한 컴포넌트로써 사용될 수 있지 않을까 싶습니다.
이런 코드조각을 공통으로 사용하기 위해서 분리하는 방법중 하나로 Compound패턴을 사용할 수 있습니다. 이 개념은 멘토링시간에 공유해보면 좋을것 같기도하네요.
컴포넌트를 만들때, 시작부터 이런것을 염두해두고 만들면 좋지만 고민해야할 부분이 많아서 오히려 오버엔지니어링 되는경우도 있기에 패턴에 대해서 한번 학습해보시고 고민해보셔도 좋을 것 같아요.
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.
ProductContainer 안에 검색 기능이 있어서 두개를 따로 만들었었는데요,
해당 기능만 따로 분리한 다음 ProductContainer를 공통 컴포넌트로 만들고 props로 best와 일반을 나누도록 수정했습니다!
src/components/common/Button.js
Outdated
@@ -0,0 +1,19 @@ | |||
import styled from "styled-components"; | |||
|
|||
const Button = styled.a` |
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.
버튼 인데 a태그로 만들어진 이유가 궁금합니다!
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.
이전 미션에서 a태그로 만들었던 걸 그대로 사용한 것 같습니다!
해당 컴포넌트는 지우고 GlobalStyle에 button에 대한 스타일을 적용하고 그 외의 부분은 그때그때 버튼 컴포넌트를 만들어서 사용하도록 변경했습니다!
`; | ||
|
||
function ProductsContainer({ screenWidth }) { | ||
const [productSorting, setProductSorting] = useRecoilState(productSortingState); |
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.
전형적인 container-presentational
패턴으로 작성된 컴포넌트 같습니다. 해당 방식은 UI와 Business코드를 적절히 분리하고 재사용하기에 용이한 구조이지만, 컴포넌트의 크기가 거대해지고 Container에 정의된 state, hook들이 점점 많아지면서 무거워지는 경향이 있습니다.
때문에 컨테이너의 크기가 커진다면 분리를 고려해야합니다. 저희팀에서는 컴포넌트의 크기가 100라인이 넘지않도록 가이드 하기도합니다.
src/utils/formatter.js
Outdated
@@ -0,0 +1,7 @@ | |||
const formatter = { | |||
formatNumber(number) { | |||
return number.toString().replace(/\B(?=(\d{3})+(?!\d))/g, ","); |
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.
사소하지만 입력값이 number가 아닐경우에 대비할수도 있어야 합니다.
물론 typescript라면 컴파일타임에 타입에러를 잡아낼 수 있지만, js에서는 그렇지않습니다. 그리고 그때 발생하는 에러는 프로젝트 전체에 크게 영향을 줄 수도 있습니다.
유효한 number의 toString()을 반환할 수 있도록 수정해주세요.
import react from '@vitejs/plugin-react-swc' | ||
|
||
// https://vitejs.dev/config/ | ||
export default defineConfig({ |
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.
++++++++++
Vite로 프로젝트를 구성해주셨네요, 스프린트미션의 가이드인지 본인의 판단인지 모르겠지만 Vite를 사용하게 된다면 어떤것들을 제공해주는지 인지하고 사용하는 자세가 필요합니다.
Vite는 모던 웹에서 가장 인기있는 라이브러리이기도 합니다. 때문에 왜 Vite를 사용해야하는지, 내부에선 방식으로 동작하는지 한번 확인해보시면 좋을 것 같아요.
@@ -0,0 +1,8 @@ | |||
import { atom } from "recoil"; | |||
|
|||
const productSortingState = 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.
전역상태를 recoil로 사용하셨군요! 리액트에서 전역상태를 다루는 방식은 매우 복잡할 수도 있습니다.
때문에 전략을 고민해보시면 좋을 것 같아요.
++ recoil은 개발팀이 해체되면서 더이상 업데이트 되고있지 않습니다 벌써 2년이 다되어가네요. 같은 개념을 사용하는 jotai라는 라이브러리로 대부분 이동하는 추세이니 확인해보세요.
+++ 상태를 관리하는 방식은 크게 3가지로 구분됩니다.
jotai, reocil(atomic 방식), zustand, redux(flux 패턴), valtio(옵저버 패턴) 이렇게 3가지로 구분될수 있는데요, 각각의 전략이 존재합니다. 본인의 서비스가 어떤 상태관리 방식이 유리한지 생각해볼 수 있어요. 여기까지는 패턴에대하여 알아야하고 또, 내부 동작방식에 대한 이해가 필요하기때문에 조금 난이도가 있을 수 있습니다. 이것도 마찬가지로 멘토링때 공유해볼 수 있을것 같네요.
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.
업데이트를 멈춘지 전혀 몰랐네요 멘토님 덕분에 새로운 걸 많이 알게됩니다 감사합니다!!
- pageSize에 값이 할당된 다음 데이터를 불러오도록 수정 - 잘못된 값 할당 수정
요구사항
기본 요구사항
공통
중고마켓 페이지
심화 요구사항
공통
중고마켓 페이지
주요 변경사항
스크린샷
배포 주소 : https://adorable-phoenix-7e2244.netlify.app/
멘토에게