[김찬희] SPRINT_MISSION_8#159
Hidden character warning
Conversation
kimchanhee0309
added
매운맛🔥
기본 요구사항공통
로그인/회원가입 페이지
로그인 페이지
회원가입 페이지
로그인, 회원가입 페이지 공통
GNB
상품 상세 페이지
심화 요구사항로그인 및 회원가입 페이지 공통
유저 기능
React-Query로 마이그레이션
로딩 및 에러 핸들링
상품 데이터 캐싱 및 업데이트
|
kimchanhee0309
added
최종제출
paran22
left a comment
paran22
left a comment
There was a problem hiding this comment.
심화 미션까지 잘 구현해주셨네요! 👍
react-hook-form도 잘 사용해주셨고, Modal과 같은 공통 ui 컴포넌트도 잘 만들어주셨네요.
폴더 구조도 고민하신게 잘 보이고, api 함수나 query key도 관리하기 쉽게 한 곳에 잘 모아놓은거 같아요.
api의 경우 인증 처리를 localstorage 통해서 처리하고 있어서요.
현재는 클라이언트에서만 api를 호출하고 있어서 괜찮은데, 미션7과 같이 서버에서 api를 호출하게된다면 어떤 구조로 가져가야할지 고민이 필요할 것 같습니다!
prefetch도 잘 사용해주셨는데요.
prefetch도 있다보니 api 호출하는 곳이 여러 곳에 있는 것 같아요.
api를 호출하는 코드는 한 곳에서 관리를 해주시는게 관리하기 좋습니다!
여러 컴포넌트나 훅에서 호출한다면 api 호출 부분 코드 확인이 필요할 때 확인해야 하는 부분이 많아지고 찾기 어려울 수 있어요.
react query 코드도 훅으로 만들어서 재사용하는 방식으로 사용하시는걸 권장합니다!
| useEffect(() => { | ||
| if (getAccessToken()) { | ||
| router.replace("/items"); | ||
| } | ||
| }, [router]); |
There was a problem hiding this comment.
RouterGuard에서 동일한 역할을 하는 로직이 있는 것 같아요! 여기서도 인증 확인이 필요한지 확인해보세요.
| const [hasToken, setHasToken] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| setHasToken(Boolean(getAccessToken())); |
There was a problem hiding this comment.
Boolean(getAccessToken())을 여러 곳에서 사용하고 있다면 아에 hasAccessToken과 같이 boolean 타입을 리턴하는 공통함수를 만드는 것도 괜찮을 것 같아요!
| router.refresh(); | ||
| router.push("/signin"); |
There was a problem hiding this comment.
어차피 페이지를 이동한다면 push만 해도 될 것 같아요!
| role="dialog" | ||
| aria-modal="true" | ||
| aria-labelledby="modal-title" | ||
| onMouseDown={(event) => event.stopPropagation()} |
There was a problem hiding this comment.
꼭 onMouseDown을 써야하는게 아니라면 onClick으로도 충분할 것 같아요! 모바일도 고려하면 onClick이 가독성 측면에서도 더 직관적일 것 같아요. 버튼도 onClick 사용하고 있구요.
| export default function Modal({ | ||
| title, | ||
| children, | ||
| confirmText = "확인", | ||
| cancelText, | ||
| onConfirm, | ||
| onClose, | ||
| danger, | ||
| busy, | ||
| }) { |
There was a problem hiding this comment.
Modal 공통 컴포넌트네요! 아주 좋습니다.
Modal은 동일한 디자인을 사용하는 경우가 많아서 보통 공통 컴포넌트로 만들어둡니다.
Modal의 버튼은 하나 혹은 두 개를 두는 경우가 많은데 버튼 텍스트는 달라질 수 있어서 onPositive + positiveText와 onNegative + negativeText와 같이 사용하기도 합니다! onNegative는 없는 경우도 있어서 옵셔널로 처리하구요.
| onClick={onConfirm || onClose} | ||
| disabled={busy} | ||
| > | ||
| {busy ? "처리 중..." : confirmText} |
There was a problem hiding this comment.
처리 중 의미라면 busy 대신 isPending도 괜찮을 것 같아요!
| const productsQuery = useQuery({ | ||
| queryKey: queryKeys.products(page, orderBy, keyword), | ||
| queryFn: () => productApi.list({ page, pageSize, orderBy, keyword }), | ||
| refetchInterval: 1000 * 30, |
There was a problem hiding this comment.
데이터를 주기적으로 업데이트할 필요가 있는 경우에는 지금 처럼 설정해주시면 될 것 같구요!
stale상태라면 페이지 이동이나, 다시 포커싱 되는 경우에도 데이터를 새로 불러와서 적절하게 설정하면 될 것 같아요.
api 요청이 너무 많아도 서버에 부하가 될 수 있어서 적절하게 설정하는 것이 필요합니다.
| if (firstProduct) { | ||
| queryClient.prefetchQuery({ | ||
| queryKey: queryKeys.product(firstProduct.id), | ||
| queryFn: () => productApi.detail(firstProduct.id), | ||
| }); | ||
| } |
There was a problem hiding this comment.
prefetch도 잘 사용하고 있네요!
특히 다음 페이지의 데이터를 미리 불러오는 건 적절하게 잘 사용한 것 같습니다.
여기는 best 상품의 첫번째 아이템을 미리 상세조회하고 있는데 첫번째 아이템을 클릭할거라는 건 확률이 높아 보이지는 않아요. prefetch가 필요한지 검토해보세요!
| {bestProductsQuery.isLoading ? ( | ||
| <div className="rounded-lg bg-[#f3f4f6] p-[18px] text-[#6b7280]"> | ||
| 상품을 불러오는 중... | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
한 페이지 내에서 best 상품과 전체 상품을 모두 조회하고 있는데 로딩 처리는 각각 처리하고 있는 것 같아요. best 상품이 위쪽에 위치하고 있어서, best 상품 조회가 전체 상품 조회보다 느릴경우 best 상품 section이 늦게 렌더링 되면서 화면 렌더링이 부자연스러울 것 같아요.
아에 둘 다 불러온 상태로 보여주거나, 위쪽에 위치한 best 상품만 먼저 확인하고 렌더링 하는 것도 고려해볼 수 있을 것 같네요.
2 participants
No description provided.