-
Notifications
You must be signed in to change notification settings - Fork 38
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단계 - 상품 목록] 에프이(박철민) 미션 제출합니다. #7
Conversation
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
- error: APIErrorFallback, ToastModal - loading: LoadingSpinner Co-Authored-By: Fe <[email protected]>
…도록 수전 Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
Co-Authored-By: Fe <[email protected]>
- 변수 이름 통일 - Toast에 전달하는 불필요한 duration prop 제거 - 이름 변경(APIErrorFallback -> APIErrorFallbackModal)
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.
안녕하세요 에프이~~~
다시 만나서 반갑습니다 ㅎㅎ 잘 지내셨나요
전체적으로 깔끔하게 잘 구현해주신거 같아요. 배포해주신 페이지는 제 브라우저에서 자동으로 https
로 redirect를 시켜서 아쉽게도 직접 사용해보진 못 했네요 ㅠㅠ
useFetch의 분리
공통으로 활용될 수 있는 로직을 적절히 잘 추상화해서 사용해 주신거 같습니다 ㅎㅎ key에 대한 질문은 코드에 코멘트로 남겼으니 확인해보시면 좋을거 같아요~
고생 많으셨습니다~~ 👍 👍 👍
src/App.tsx
Outdated
return idx + 1 !== products.length ? ( | ||
<ProductItem product={product} key={`${product.id}_${idx}`} /> | ||
) : ( |
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만 다르게 render해주려고 한거 같은데요. 코드만 보면 직관적으로 이해되지 않는거 같아요. 조금 더 읽기 쉬운 코드로 만들어 보시면 좋을거 같습니다!
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.
{products.map((product, idx) => {
const isLastProduct = idx + 1 === products.length;
return isLastProduct ? (
<IntersectionArea onImpression={fetchNextPage} key={`${product.id}_${idx}`}>
<ProductItem product={product} />
</IntersectionArea>
) : (
<ProductItem product={product} key={`${product.id}_${idx}`} />
);
})}
마지막 product인지를 확인하는 로직을 변수로 분리해서, 직관적으로 이해될 수 있게 바꾸어 보았습니다.
1ef190e
질문
조건부 렌더링을 할 때, 긍정인 경우를 먼저 처리하는 것을 권장하시는지 궁금합니다! isLastProduct
인 경우 3줄의 코드를 렌더링하고, 그렇지 않는 경우 1줄의 코드를 렌더링하게 되는데, 만약 긍정인 경우에 렌더링해야 하는 코드가 매우 길어지는 경우를 생각해보았을 때 코드의 가독성이 떨어지지는 않을지 고민이 들었습니다.
예를 들어,
return isLastProduct ? (
<IntersectionArea onImpression={fetchNextPage} key={`${product.id}_${idx}`}>
<ProductItem product={product} />
<div>렌더링할 코드</div>
<div>렌더링할 코드</div>
<div>렌더링할 코드</div>
// ...
<div>렌더링할 코드</div>
</IntersectionArea>
) : (
<ProductItem product={product} key={`${product.id}_${idx}`} />
);
이런 경우에는 !isLastProduct
일 때를 먼저 렌더링한다면 짧은 코드를 먼저 확인할 수 있어서 코드를 파악하기 더 쉬울 것 같습니다.
return !isLastProduct ? (
<ProductItem product={product} key={`${product.id}_${idx}`} />
) : (
<IntersectionArea onImpression={fetchNextPage} key={`${product.id}_${idx}`}>
<ProductItem product={product} />
<div>렌더링할 코드</div>
<div>렌더링할 코드</div>
<div>렌더링할 코드</div>
// ...
<div>렌더링할 코드</div>
</IntersectionArea>
);
좀 더 읽기 쉬운 코드를 만드는 과정에서 든 사소한 생각이었습니다😊
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/App.tsx
Outdated
) : ( | ||
<IntersectionArea | ||
onImpression={fetchNextPage} | ||
key={`${product.id}_${idx}`} |
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.
위의 고민한 내용에서 언급했듯이, 현재 서버가 데이터를 제공하는 방식에 맞추어 적절하게 유일한 key값을 가질 수 있도록 인덱스와 product.id를 조합해서 사용하고 있습니다. key값을 설정할 때, 프론트 단에서 알아서 유일할 수 있는 값을 만드는 것이 일반적인지, 혹은 백엔드 측과 논의를 통해 특정 필드를 사용하는 것이 일반적인지 궁금합니다.
이게 PR 본문에서 남겨주신 질문이군요 ㅎㅎ
보통 일반적으로는 서버에서도 특정 DB테이블에 대한 PK(Primary Key)를 가지고 있고, 해당 값을 같이 내려주기 때문에 이런 일이 자주 발생하지는 않는데요. 지금 케이스는 서버의 응답이 조금 이상한 상황으로 보입니다 ㅎㅎ
이럴 때는 지금처럼 index
를 사용하는 방법도 있고, 다른 value
들을 가지고 조합해서 사용하거나 uuid
같은 라이브러리를 이용해 key값을 만들어서 넣어주기도 합니다
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/api/cart.ts
Outdated
import { HEADERS } from './common'; | ||
import { CART_ITEMS_ENDPOINT } from './endpoints'; | ||
|
||
export const fetchCartItems = async () => { |
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.
any타입으로 보였네요😅
CartItem[]
형을 반환함을 명시했습니다!
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.
svg를 .svg
가 아닌 .tsx
확장자로 관리하시는 이유가 있을까요?
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.
svg
태그를 반환하는 컴포넌트로 사용하기 위함입니다!
props를 받아서 벡터 이미지의 색상을 변경해야 하는 경우 등에 유연하게 대처할 수 있다는 장점이 있다고 생각합니다.
이 밖에도 내부에서 상태를 관리할 수 있고, 코드를 관리하기 편하다는 장점도 있을 것 같습니다.
border: 1px solid ${({ theme }) => theme.colors.optionBorder}; | ||
&:focus, | ||
&:hover { | ||
border: 1px solid ${({ theme }) => theme.colors.black}; |
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.
theme
을 많이 사용하고 있어서 바로 import 해서 사용해도 괜찮을거 같네요
border: 1px solid ${({ theme }) => theme.colors.black}; | |
border: 1px solid ${theme.colors.black} |
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.
질문
ThemeProvider를 사용해서 props로 객체를 사용하는 방법만 알고 있었는데, 바로 import해서 사용할 수도 있겠네요! 코드가 굉장히 간결해져서 좋지만, 이렇게 되면 emotion에서 제공하는 ThemeProvider를 활용하는 느낌이 아닌, 단순히 외부 상수를 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.
오 제가 한번도 생각하지 못 했던 부분이네요 👍 👍 👍
저는 CSS in JS를 활용하는 장점 중 하나가 JavsScript의 변수를 활용할 수 있는 점이라고 생각해서 크게 거부감 없이 위에 제안드린 방법을 사용했던거 같습니다 ㅎㅎ
저는 CSS in JS의 장점을 이용하는거라 문제가 없다고 생각하지만 emotion에서 제공하는 방법을 그대로 따르고 싶다면 기존 코드대로 유지해도 좋을거 같네요!
background-color: ${({ isPushed, theme }) => | ||
isPushed ? theme.colors.btnActiveBgColor : theme.colors.btnBgColor}; | ||
color: ${({ isPushed, theme }) => | ||
isPushed ? theme.colors.black : theme.colors.white}; |
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에서 prop에 따른 분기를 처리할 때 아래와 같은 Mapper 형식도 많이 사용하니 참고해보셔도 좋을거 같아요 ㅎㅎ(변경하라는 얘기는 아닙니다)
export const _Box = styled.div(
(props: BoxProps) => css`
display: flex;
gap: ${props.gap}px;
${alignMapper(props.direction)}
`
);
const alignMapper = (direction: StyledBoxType['direction']) => {
switch (direction) {
case 'vertical':
return css({
flexDirection: 'column',
});
case 'horizontal':
default:
return css({
flexDirection: 'row',
});
}
};
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에 따라 여러 가지 속성의 값이 분기 처리될 때 Mapper가 아주 유용할 것 같네요!
감사합니다 :)
function HomeButton() { | ||
const navigate = useNavigate(); | ||
const moveToHome = () => { | ||
navigate(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.
navigate(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.
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.
해당 내용은 직관적으로 알 수 있는데 왜 홈으로 이동(moveToHome)을 하는데 새로고침을 하는지는 잘 모르겠네요 😵💫
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.
다른 페이지에도 같은 Header가 재사용될 때 새로고침 로직이 있다면 moveToHome
이 아니게 되네요😅 메인 페이지의 path로 navigate되도록 수정했습니다 :)
import Logo from '../../../assets/images/logo.png'; | ||
import { useNavigate } from 'react-router-dom'; | ||
import { useContext } from 'react'; | ||
import { CartItemsContext } from '../../../context/CartItemsProvider'; |
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.
이번에 절대 경로를 처음으로 적용해보았는데, depth가 깊어지는 상대 경로의 경우 가독성이 많이 좋아진 것 같습니다. 같은 폴더 안에 있거나 바로 부모 폴더에 있는 컴포넌트를 import하는 경우에는, vscode가 상대 경로와 절대 경로를 섞어서 더 효율적인 방식으로 auto import하는 것 같습니다!👍
@@ -0,0 +1,17 @@ | |||
import { CommonQueryParams } from '../types/fetch'; | |||
|
|||
export const generateQueryParams = <T extends CommonQueryParams>( |
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.
해당 로직은 query string을 만들어주는 함수입니다.
전체 카테고리를 선택하면 category
상태 값으로 ''
를 갖게 되는데, 이 때 요청 URL에는 category
파라미터 자체가 포함되지 않습니다. 이런 경우를 대비해서 값이 존재하는 파라미터만 사용해서 query string을 만들도록 util 함수로 분리했습니다.
setRefresh: React.Dispatch<SetStateAction<boolean>>; | ||
} | ||
|
||
export const CartItemsContext = createContext<CartItemsContextValue>({ |
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.
Provider로 감싸져있지 않은 곳에서 이 Context를 호출하면 어떤 일이 발생할까요?
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.
해당 context의 값이 undefined
로 설정되는 것으로 알고 있습니다.
undefined
를 읽게 되면 예상치 못한 동작이 일어날 수 있기 때문에 다음과 같이 처리해 주었습니다.
f123b4f
const { cartItems } = useContext(CartItemsContext) || { cartItems: [] };
const { cartItems, setRefresh } = useContext(CartItemsContext) || { cartItems: [], setRefresh: () => {} };
안녕하세요 유조! 피드백 반영을 마치고 돌아왔습니다. 🌐배포 주소🔧수정 사항절대 경로 사용
theme에서 font 관련 속성 관리
그 밖의 내용들
❓질문
|
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.
안녕하세요 에프이~~
코멘트 반영하느라 고생 많으셨습니다! 에프이가 고민하고 있는 부분들이나 코드를 작성할 때 고려하고 있는 부분들에서 제가 오히려 배워가는 점들이 있던 리뷰였네요 ㅎㅎ 👍 👍 👍
지난 코멘트에 남겨주신 질문에 대한 답변과 더불어 소소한 코멘트들 남겼으니 확인해보시며 좋을거 같습니다!
이번 step은 여기서 이만 merge하고 다음 step으로 넘어가면 좋을거 같네요~!
고생 많으셨습니다 에프이 step2에서 다시 만나용~~~ 👍 👍 👍
onImpression={fetchNextPage} | ||
key={`${product.id}_${idx}`} | ||
> | ||
const isLastProduct = idx + 1 === products.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.
오 이렇게 변수로 분리만 해도 가독성이 훨씬 좋아지네요 ㅎㅎ 👍 👍 👍
src/components/hooks/useProducts.ts
Outdated
if (isLastPage) return; | ||
|
||
if (page === START_PAGE_NUMBER) setPage(page + INITIAL_PAGING_SIZE / PAGING_SIZE); | ||
else setPage(page + 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.
한결 보기 좋네요 ㅎㅎ 👍
import { generateQueryParams } from '../utils/generateQueryParams'; | ||
import { HEADERS } from './common'; | ||
import { CART_ITEMS_ENDPOINT } from './endpoints'; | ||
|
||
export const fetchCartItems = async () => { | ||
export const fetchCartItems = async (): Promise<CartItem[]> => { |
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.
👍 👍 👍
@@ -1,9 +1,9 @@ | |||
import styled from '@emotion/styled'; | |||
|
|||
export type Size = 'small' | 'large'; | |||
export type Size = 'default' | 'full'; |
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.
👍 👍 👍
"noFallthroughCasesInSwitch": true, | ||
|
||
/* Path */ | ||
"baseUrl": "src", |
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.
👍 👍 👍
@@ -34,7 +34,7 @@ export default function useProducts(): UseProductsResult { | |||
const [category, setCategory] = useState<string>(''); | |||
const [isLastPage, setIsLastPage] = useState<boolean>(false); | |||
|
|||
const getProducts = useCallback(async () => { | |||
const getProducts = async () => { |
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 fontSize: FontSize = { | ||
title: '24px', | ||
description: '12px', | ||
normal: '14px', | ||
|
||
cartIcon: '10px', | ||
}; | ||
|
||
const fontWeight: FontWeight = { | ||
bold: '700', | ||
semibold: '600', | ||
medium: '500', | ||
normal: '400', | ||
}; |
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.
👍 👍 👍
안녕하세요 유조! 에프이예요. 레벨 1 첫 미션에 함께 했었는데 레벨 2 마지막 미션도 함께 하게 되어 너무 반갑습니다😊
🌐배포 주소
바로가기
안전하지 않은 콘텐츠
를허용
으로 바꿔주세요!📚구조
파일 구조 트리 보기
📦src
┣ 📂api
┃ ┣ 📜cart.ts
┃ ┣ 📜common.ts
┃ ┣ 📜endpoints.ts
┃ ┗ 📜products.ts
┣ 📂assets
┃ ┗ 📂images
┃ ┃ ┣ 📜cartIcon.png
┃ ┃ ┗ 📜logo.png
┣ 📂components
┃ ┣ 📂common
┃ ┃ ┣ 📂APIErrorFallbackModal
┃ ┃ ┃ ┗ 📜index.tsx
┃ ┃ ┣ 📂Dropdown
┃ ┃ ┃ ┣ 📜index.tsx
┃ ┃ ┃ ┗ 📜style.ts
┃ ┃ ┣ 📂IntersectionArea
┃ ┃ ┃ ┗ 📜index.tsx
┃ ┃ ┣ 📂LoadingSpinner
┃ ┃ ┃ ┗ 📜style.ts
┃ ┃ ┣ 📂Title
┃ ┃ ┃ ┣ 📜index.tsx
┃ ┃ ┃ ┗ 📜style.ts
┃ ┃ ┣ 📂ToastModal
┃ ┃ ┃ ┣ 📜index.tsx
┃ ┃ ┃ ┗ 📜style.ts
┃ ┃ ┗ 📜Arrows.tsx
┃ ┣ 📂hooks
┃ ┃ ┣ 📜useCartItems.ts
┃ ┃ ┣ 📜useFetch.ts
┃ ┃ ┣ 📜useProducts.test.ts
┃ ┃ ┗ 📜useProducts.ts
┃ ┣ 📂layout
┃ ┃ ┣ 📜index.tsx
┃ ┃ ┗ 📜style.ts
┃ ┗ 📂product
┃ ┃ ┣ 📂CartButton
┃ ┃ ┃ ┣ 📜Icons.tsx
┃ ┃ ┃ ┣ 📜index.tsx
┃ ┃ ┃ ┗ 📜style.ts
┃ ┃ ┣ 📂Header
┃ ┃ ┃ ┣ 📜index.tsx
┃ ┃ ┃ ┗ 📜style.tsx
┃ ┃ ┣ 📂ProductItem
┃ ┃ ┃ ┣ 📜index.tsx
┃ ┃ ┃ ┗ 📜style.ts
┃ ┃ ┗ 📂ProductList
┃ ┃ ┃ ┣ 📜index.tsx
┃ ┃ ┃ ┗ 📜style.ts
┣ 📂constants
┃ ┣ 📜api.ts
┃ ┣ 📜filterOptions.ts
┃ ┗ 📜page.ts
┣ 📂context
┃ ┗ 📜CartItemsProvider.tsx
┣ 📂mocks
┃ ┣ 📜handlers.ts
┃ ┣ 📜products.json
┃ ┗ 📜server.ts
┣ 📂router
┃ ┗ 📜AppRouter.tsx
┣ 📂styles
┃ ┣ 📜globalStyles.ts
┃ ┗ 📜theme.ts
┣ 📂types
┃ ┣ 📜cartItem.ts
┃ ┣ 📜fetch.ts
┃ ┣ 📜index.ts
┃ ┗ 📜product.ts
┣ 📂utils
┃ ┗ 📜generateQueryParams.ts
┣ 📜App.tsx
┣ 📜main.tsx
┗ 📜vite-env.d.ts
컴포넌트 구조도 보기
❗기능 구현 사항
useContext를 사용한 상태 관리
delete
요청을 보낼 때 장바구니 아이템의 id 값이 필요합니다.무한 스크롤
에러 Toast, 로딩 스피너
loading
상태를 보내서 로딩 중인 경우 로딩 스피너를 렌더링합니다.error
상태를 보내서 api 요청 시 에러가 발생한 경우 에러 Toast를 렌더링합니다.중복 데이터 수신
ProductItem
의 key값을 product.id로 설정했을 때 유일하지 않은 문제가 있었습니다.{인덱스}_{product.id}
형식으로 key를 설정해서 이 문제를 해결했습니다.❓질문
key값 설정
useFetch의 분리
감사합니다 😊