Skip to content

[6주차] Team 프로메사 권동욱 & 김서연 미션 제출합니다. #11

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

Open
wants to merge 166 commits into
base: master
Choose a base branch
from

Conversation

2025314242
Copy link

@2025314242 2025314242 commented May 14, 2025

마치며...

👨‍💻 권동욱

  • 5주차에 기본적인 과제 로직은 구현을 완료했었기에, 이번 주차는 조금 편한 마음으로 최적화를 진행했다.
  • Key questions를 보다가 TanStack Query라는 라이브러리를 알게 되었다. 원래는 캐싱을 위해서 zustand 라이브러리를 사용했었는데, 생각해보니까 해당 라이브러리가 product store라는 기능과 핏이 맞지 않는 거 같아서 TanStack Query 라이브러리를 적용했다. 해당 라이브러리 특성 상 서버-사이드와 클라이언트-사이드가 명확하게 구분되지 않고 코드가 혼용될 경우 에러가 발생하는데, 관련 각종 에러를 핸들링하는 과정에서 SSR이랑 CSR에 대해 좀 더 잘 알게 되었다. 좀 더 UX를 높이기 위해서는 핵심 이미지만 서버-사이드에서 API 호출해서 먼저 띄워주고, 다른 이미지들은 클라이언트-사이드에서 가져오는 방식도 괜찮을 것 같다. 근데 또 생각해보면 next/dynamic 사용해서 lazy loading하는 게 맞는 거 같기도 하고; 추후 과제 및 프로젝트 진행하면서 여러 가지 테스트해보고자 한다.
  • Tailwind CSS로 폰트 스타일이나 주요 색상 등을 전역적으로 선언했었는데, 타이핑 실수 (--로 구분지어야 하는데 - 하나만 적음) 로 font-size 외 다른 스타일이 적용이 되어있지 않았었다.

👩‍💻 김서연

  • 저번 주에 무한스크롤 형태로 검색 페이지를 구현해 놓아서 코드리뷰 기반 리팩토링을 중심으로 진행했습니다.
  • 기존에는 버튼 클릭 시에만 검색이 가능했으나, 실시간 검색 기능으로 개선하였습니다. 다만 검색어가 바뀔 때마다 매번 API가 호출되는 문제가 아직 남아 있어서 이번 리뷰 피드백 남겨주신 것 참고해서 수정해보겠습니다!
  • Tanstack query를 사용해 무한 스크롤을 리팩토링하였고, 기존 상태 관리만으로 구현했을 때 복잡하던 코드를 깔끔하게 정리할 수 있었습니다.

배포 링크

https://next-netflix-21th-promesa.vercel.app

Key Questions

1️⃣ 정적 라우팅 (Static Routing) vs 동적 라우팅 (Dynamic Routing)

정적 라우팅 (Static Routing)

정적 라우팅 (Static Routing) 은 파일 시스템 기반으로 URL이 고정된 구조를 따르는 라우팅 방식이다. 정적 라우팅은 단순하고 명확하며, 빌드 시점에 각 페이지가 정적으로 생성될 수 있기 때문에 성능 측면에서 유리하다.

src/app/about/page.tsx    # /about
src/app/contact/paget.tsx # /contact

동적 라우팅(Dynamic Routing)

동적 라우팅 (Dynamic Routing) 은 경로 내에서 변수나 패턴을 사용하여 다양한 URL에 대응하는 라우팅 방식이다. 동적 라우팅은 사용자 또는 데이터에 따라 동적으로 페이지를 생성할 수 있어 유연성과 확장성 측면에서 뛰어난 장점을 가진다.

src/app/blog/[id]/page.tsx          # /blog/1, /blog/2 ...
src/app/[username]/profile/page.tsx # /john/profile, /jane/profile ...

2️⃣ 무한 스크롤 & Intersection Observer API

무한 스크롤

무한 스크롤은 사용자가 페이지를 스크롤할 때 일정 지점에 도달하면 자동으로 다음 데이터를 로드하여 지속적으로 콘텐츠를 표시하는 UX 패턴이다. 대표적으로 SNS 피드나 상품 목록 페이지 등에서 활용된다.

Intersection Observer API

브라우저에서 특정 요소가 뷰포트에 진입하거나 이탈하는 시점을 비동기적으로 감지할 수 있는 API로, 무한 스크롤 구현 시 스크롤 하단 감지에 사용된다. 기존의 scroll 이벤트를 사용하는 방식에 비해 퍼포먼스가 뛰어나며, 리소스를 적게 소모하고 코드가 간결하다.

무한 스크롤 구현 흐름은 다음과 같다:

  1. 마지막 아이템 하단에 ref를 부여한 요소를 렌더링한다.
  2. Intersection Observer로 해당 요소가 보이는 시점을 감지한다.
  3. 추가 데이터를 비동기적으로 fetch한다.
  4. 기존 리스트에 append한다.

3️⃣ TanStack Query (React Query)

TanStack Query는 서버 상태를 효율적으로 관리하기 위한 데이터 패칭 라이브러리다. 기존의 클라이언트 상태 중심 라이브러리들 (e.g. Recoil, Redux) 은 주로 UI 상태 또는 로컬 상태를 다루는 데 초점이 맞춰져 있지만, TanStack Query는 서버에서 가져온 데이터를 캐싱하고 자동으로 갱신하는 기능을 중심으로 설계되었다.

주요 특징은 다음과 같다:

  • 동일한 queryKey에 대해 데이터를 재요청하지 않으며, staleTime 설정을 통해 일정 시간 내 캐싱된 데이터를 재사용한다.
  • background fetch와 refetchOnWindowFocus 기능으로 항상 최신 데이터를 유지할 수 있다.
  • 쿼리 상태에 따른 loading, error, success 등의 상태 관리가 내장되어 있어, 별도의 상태 관리 코드를 줄일 수 있다.
  • SSR/SSG와 호환되는 dehydratedState 제공으로 Next.js 환경에서도 효율적인 서버 상태 전달 및 복원이 가능하다.

사용 예시

React에서는 CSR만 지원하며, QueryClientProvider로 전체 앱을 감싸고, useQuery/useMutation 훅을 통해 데이터를 선언적으로 요청할 수 있다.

const { data } = useQuery({
  queryKey: ['products'],
  queryFn: fetchProducts,
});

Next.js에서는 SSR과 CSR을 혼합 지원하고, 서버 측 prefetchQuery + dehydrate를 통해 SSR에 데이터를 미리 담아 전달한 뒤, 클라이언트에서 HydrationBoundary를 통해 복원하는 방식으로 사용한다.

// server
await queryClient.prefetchQuery(...)
const dehydratedState = dehydrate(queryClient)

// client
const { data } = useSuspenseQuery({ queryKey, queryFn })

@Programming-Seungwan
Copy link

안녕하세요! 코드 리뷰를 하려고 하는데, 혹시 환경변수 파일이나 gitignore에 포함된 파일들이 있을까요? 있으시다면 slack 채널에 남겨주시면 감사하겠습니다 :)

@2025314242
Copy link
Author

안녕하세요! 코드 리뷰를 하려고 하는데, 혹시 환경변수 파일이나 gitignore에 포함된 파일들이 있을까요? 있으시다면 slack 채널에 남겨주시면 감사하겠습니다 :)

DM으로 보내드렸습니다

Comment on lines +21 to +28
"baseUrl": ".",
"paths": {
"@public/*": ["public/*"],
"@app/*": ["src/app/*"],
"@components/*": ["src/components/*"],
"@models/*": ["src/models/*"],
"@services/*": ["src/services/*"]
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

타입스크립트를 이용해서 상대 경로가 아니라 절대 경로를 잘 이용해주시고 계시네요. 혹시 react 프로젝트를 vite로 만들어보셨나요? 이떄에는 tsconfig.json 파일 뿐만 아니라 해당 설정을 vite.config.ts 파일에도 해주어야 하는데요, 번들러와 타입스크립트의 동작 원리에 대해서도 한번씩 생각해보시면 좋을 것 같아요.
코드를 번들러가 해석하느냐, 타입스크립트 컴파일러가 해석하느냐에 따라서 이 설정도 필요한 건데 기본적으로 nextJS는 swc라는 트랜스파일러를 이용해서 내부적으로 webpack으로 빌드를 하기 때문에 따로 번들링을 위한 상대 경로 설정은 할 필요가 없는 것으로 알고 있어요. 이 부분 한번 짚고 넘어가시면 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vite를 사용해본 적은 없습니다만, 관련해서 한 번 정리해보겠습니다 감사합니다

Copy link

@Programming-Seungwan Programming-Seungwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

과제 진행하시느라 정말 고생 많으셨습니다.

Comment on lines +1 to +14
import IndicatorBar from '@components/indicator-bar';

export default function PublicLayout({
children,
}: Readonly<{
children: React.ReactNode;
}>) {
return (
<>
{children}
<IndicatorBar />
</>
);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 해당 프로젝트를 보면 라우트 그룹이 Public과 routes로 나뉘는 것 같아요. 처음 랜딩 페이지와 다른 페이지 간에 nav bar가 붙고 말고 때문에 이렇게 하신 것 같습니다.
물론 layout.tsx에 네브 바를 넣으면 불필요한 리렌더링을 줄일 수 있지만 혹시나 routes 그룹에 바로 page.tsx를 넣으면 루트 경로에 대해 라우트 충돌이 나겠죠? 이런 점은 어떻게 해결할지 고민해보시면 좋겠습니다.

await queryClient.prefetchQuery({
queryKey: ['tmdb', thumbnailPath],
queryFn: async () =>
(await fetch(`${process.env.NEXT_PUBLIC_BASE_URL}${thumbnailPath}`, { cache: 'no-store' })).json(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서버에서 쿼리 클라이언트로 prefetch하여 서버로 던져주는 로직을 매우 잘 이해하고 계신 것 같습니다. 다만 Fetching한 데이터를 Json으로 직렬화하고 계신데 axios를 쓰면 이게 자동으로 되어서 사용을 고려해보셔도 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안 그래도 운영진 분에게 axios 관련하여 조언 들어서 다음 과제부터는 적용해보려 합니다 감사합니다

Comment on lines +21 to +24
const { data } = useSuspenseQuery<ThumbnailData>({
queryKey: ['tmdb', path],
queryFn: async () => (await fetch(`${process.env.NEXT_PUBLIC_BASE_URL}${path}`)).json(),
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러를 밖으로 던지는 useSuspenseQuery 훅을 잘 사용하신 것 같네요. 그냥 useQuery()를 이용하셨으면, isLoading, isFetchingError 등의 처리를 다 해주셔야 하는데 한번에 처리하는 코드 좋네요

Comment on lines +1 to +9
export const sectionItems = [
{ title: 'Previews', apiPath: '/api/products/popular', option: 'previews' },
{ title: 'Top 10 in Worldwide Today', apiPath: '/api/products/trending-top10' },
{ title: 'Korean Movies', apiPath: '/api/movies/korean' },
{ title: 'Netflix Originals', apiPath: '/api/tv-shows/netflix-originals', option: 'netflix-originals' },
{ title: 'New Releases', apiPath: '/api/products/new-releases' },
{ title: 'TV Mysteries', apiPath: '/api/tv-shows/mystery' },
{ title: 'KR TV Shows', apiPath: '/api/tv-shows/korean' },
];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 코드를 보시면 스타일링을 위해서 option 속성을 받고 있어요. 그런데, 이미 title에 다 있는 정보인데 굳이 optional 한 데이터로 option 필드를 넣는게 좋을까요?
그리고 product-list 컴포넌트에서 정의한 interface를 이 상수에 정의해주셔도 좋을 것 같습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

또한 constants 디렉터리에 들어가는 상수인만큼, CONST_SECTION_ITEMS 라는 이름으로 네이밍해주시고, 디렉터리 내부에 index.ts 파일 만드셔서 barrel 방식으로 여기 저기서 끌어쓸 수 있게하면 더 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

property interface 같은 경우는 기본적으로 각 컴포넌트 파일 내에 선언하는 걸 선호하는 편이어서 그렇게 작성하였고, option 속성 같은 경우는 현재는 title이랑 거의 흡사하지만 추후 숫자나 다른 키워드와 같이 title과 직접적인 관련 없는 데이터로 바뀔 수도 있기 때문에 다음과 같이 설정하였습니다.

네이밍 관련 부분은 대문자로 설정하여 구분할 수 있도록 하겠습니다. index.ts 같은 경우는 안 그래도 어느 폴더(e.g. constants)에 넣고 어느 폴더(e.g. components)에 안 넣을지 고민 중에 있었는데, 해당 부분 좀 더 고민해보겠습니다 감사합니다

</body>
</html>
);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 파일에 관한 내용은 아닌데, 사용자가 그냥 이상하게 /ㅁㄴㅇㄹㄴ이ㅓ;ㅁㄴㅇ;ㅣ런ㅁ이ㅏㅓ 같은 경로로 들어왔을 떄 404 페이지가 그냥 nextJS가 기본으로 제공해주는 UI가 뜨네요.
요즘 디자인 인공지능 좋은 것도 많으니까 간단하게 제작하여서 not-found.tsx 파일에 넣어주면 더 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안 그래도 loading/not-found/error.tsx 등 관련하여 이번 과제에 추가하지 못한 것을 아쉽게 생각하고 있어서, 추후 과제 및 프로젝트에는 좀 더 완성도 높은 코딩 진행하고자 합니다 감사합니다

return (
<nav className="bg-background-01 absolute bottom-[27px] z-990 flex h-12 w-full justify-between pt-2">
{navItems.map(({ href, label, Icon }) => {
const isActive = pathName.startsWith(href);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 지금 navBar가 공통적인 UI라 layout.tsx에 있어서 리렌더링이 페이지 이동을 해도 안되는 상황이죠? 이건 page.tsx 파일에서 전달해줄 수 있는 부분이 아니니, 클라이언트 컴포넌트로 잘 선정하시고 usePathname() 훅을 잘 이용해주셨네요. 다만 startsWith 보다는 /로 나눠서 index가 1인 컴포넌트가 href와 비교해서 isActive 변수에 넣어도 좋았을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

백슬래쉬로 경로 split해서 체크하라는 말씀이시죠? 해당 방안이 확실히 더 strict하게 확인할 수 있는 방안인 것 같습니다. 안 그래도 nav-bar와 같이 여러 페이지에 공통적으로 나타나는 요소를 어떻게 구성할 지 고민 중이었는데 참고하겠습니다 감사합니다

const trimmedKeyword = keyword.trim();

// 무한스크롤 감지용 intersectionObservation
const { ref, inView } = useInView();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 개꿀 훅이 있는건 처음 알았네요. 저는 항상 그냥 내부 브라우저 크기랑 전체 문서 비교해서 이거 구현했는데,,,,, 잘 쓰겠습니다!

Comment on lines +56 to +59
const handleSearch = (searchText = '') => {
setKeyword(searchText);
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 제어 컴포넌트, 비제어 컴포넌트의 로직과도 연결이 되는데 현재 search input 박스에 타이핑을 할 때마다 keyword 상태가 바뀌고 리렌더링 되면서 api 요청이 발생하고 있네요.
쓰로틀링이나 디바운싱 로직을 Lodash 라이브러리 이용해서 걸어주거나, 아니면 useRef 를 이용한 비제어 컴포넌트 방식을 이용해서 사용자가 엔터를 친 시점에만 한번 api 콜이 발생하도록 바꿔줘도 좋겠습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안 그래도 이 부분 어떻게 해결하면 좋을지 고민 중이었는데 좋은 피드백 감사합니다!! 참고해서 수정해 보겠습니다😄

Comment on lines +1 to +10
'use client';

import { ReactNode, useState } from 'react';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';

export default function Providers({ children }: { children: ReactNode }) {
const [queryClient] = useState(() => new QueryClient());

return <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

루트 레이아웃에서 QueryClientProvider로 이미 전체 애플리케이션을 래핑해주신 것 같은데, 굳이 여기에서 한번 더 작성하시고 또 하실 필요가 있을까요? 이렇게 되면 useState로 쿼리 클라이언트 상태도 쓸데 없이 2개라서 싱글턴 패턴도 깨질 수도 있구요

Comment on lines +18 to +22
<img
src={product.image ?? '/default-img.png'}
alt={product.name ?? 'no name'}
className="h-[76px] w-[146px]"
/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image 컴포넌트로 최적화 해줍시다

Copy link

@gustn99 gustn99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

탠스택 쿼리 관련해서는 모르는 내용이 많아 전반적인 넥스트 및 스타일 관련 피드백 남겼습니다. 과제 잘 봤습니다. 고생 많으셨어요!

Comment on lines +13 to +34
const eslintConfig = [
...compat.extends(
'next/core-web-vitals',
'next/typescript',
'plugin:prettier/recommended',
'plugin:@tanstack/eslint-plugin-query/recommended',
),
{
plugins: {
'simple-import-sort': simpleImportSort,
},
rules: {
'simple-import-sort/imports': [
'error',
{
groups: [['^\\u0000'], ['^node:'], ['^react', '^@?\\w'], ['^(@|~)/'], ['^\\.'], ['\\.s?css$']],
},
],
'simple-import-sort/exports': 'error',
},
},
];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 간단하게 import 설정을 할 수 있어서 좋네요! 저는 eslint plugin import밖에 사용을 안 해 봤는데 자체 내장 group을 기반으로 커스텀하는 방식이라 복잡해지는 감이 있더라고요. react처럼 next 프로젝트에서는 next 관련 import문을 상단으로 올리는 것도 좋은 옵션일 것 같습니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

가변 폰트를 지원하는 폰트라면 하나의 파일로 여러 스타일을 커버할 수 있어 유연한 설정이 가능합니다! 프로젝트에서 이런 부분 고려해 진행하시면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다! SF Pro 폰트는 찾아보니 variable을 도저히 못 찾겠어서 이렇게 따로따로 저장했습니다 ㅜㅜ

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

슬픈 일이네요 ,,,😢

Comment on lines +9 to +16
return (
<>
{children}
<NavBar />
<IndicatorBar />
</>
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public과 routes 그룹 모두 IndicatorBar를 가지는 것 같습니다. 이런 경우에는 루트 layout에 IndicatorBar를 두고, NavBar를 가지는 라우트 그룹만 추가하는 방법도 유효했을 것 같네요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 안그래도 이 부분은 추후 과제 및 프로젝트 진행할 때 분기 형식으로 처리하고자 합니다!

Comment on lines +1 to +19
@font-face {
font-family: 'SF Pro Display';
font-style: normal;
font-weight: 400;
font-display: swap;
src: url('/fonts/sf-pro-display-regular.woff2') format('woff2');
}

@font-face {
font-family: 'SF Pro Display';
font-style: normal;
font-weight: 500;
font-display: swap;
src: url('/fonts/sf-pro-display-medium.woff2') format('woff2');
}

@font-face {
font-family: 'SF Pro Display';
font-style: normal;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 css 방식으로 font face를 설정하셨네요! next에서는 공식 문서에서도 next/font/local의 localFont를 사용하고 있습니다. 성능 최적화 등의 측면으로 localFont가 유리하다고 하니 한번 찾아보시면 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와 감사합니다 !!!

Comment on lines +20 to +38
return (
<section className="hide-scrollbar h-full overflow-x-hidden overflow-y-auto pb-[85px]">
<div className="relative left-[-24.52px] h-[415px] w-[424.05px]">
{item.image && item.name && (
<Image src={item.image} alt={item.name} fill sizes="424.05px" className="object-cover" priority />
)}
<div className="absolute inset-0 bg-[linear-gradient(180deg,rgba(0,0,0,0.45)_0%,rgba(0,0,0,0)_87.26%,#000_100%)]" />
</div>
<div className="bg-background-03 mt-[13px] mb-8 ml-9 flex h-[45px] w-[303px] items-center justify-between rounded-[5.63px] pt-2 pr-[111px] pb-[7px] pl-[120px]">
<PlayIcon className="text-grayscale-00-black h-[21.6px] w-[18px]" />
<span className="text-subhead-01 text-grayscale-00-black">Play</span>
</div>
<div className="mx-8 flex flex-col gap-6">
<span className="text-headline-01 text-grayscale-02-white">{item.name}</span>
<span className="text-body-04 text-grayscale-02-white-tp wrap-break-word break-all">{item.description}</span>
</div>
</section>
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체적으로 고정 width heigth를 많이 사용하신 것 같습니다. 피그마 화면을 정말 있는 그대로 옮겼기 때문인 것 같은데요, 프로젝트에서는 단순히 화면을 그대로 구현하는 것보다도 함께 개발하는 개발자가 확인하기 쉽도록, 유지보수하기 쉽도록 구현하는 데 신경 써 보시면 어떨까 싶습니다.

특히나 반응형 작업을 위해서는 레이아웃 관련 width나 height에 고정값을 최대한 피하는 편이 좋습니다. 적절한 패딩이나 마진을 주고 width는 full이나 grow로 주는 식으로요!

또 지금은 좌우 여백을 맞추기 위해 좌측에 마진을 준다거나 하는 식으로 작업이 되어 있는데, 가운데 정렬을 위해 어떤 클래스들을 사용할 수 있는지도 공부해 보시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞습니다 ㅜㅜ 안 그래도 추후 진행되는 프로젝트에서는 최대한 가변 값으로 작업하고자 합니다 감사합니다@!

Comment on lines +4 to +5
return (
<header className="absolute top-6 z-900 flex w-full items-center justify-between pr-[18px] pl-[19px]">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고정 width height 관련 피드백에 이어서, 이런 패딩도 px-[18px]로 처리해서는 안 되는 이유가 있었을까 의문이 드는 지점인 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞습니다 이번 과제에서는 피그마 디자인을 그대로 따라하느라 저렇게 좌우간격이 안 맞는 지점이 있었는데 앞으로는 해당 부분 유연하게 처리하고자 합니다!

Comment on lines +20 to +29
const isPreview = option === 'previews';
const isNetflixOriginal = option === 'netflix-originals';

const heightClass = isPreview ? 'h-[102px]' : isNetflixOriginal ? 'h-[251px]' : 'h-[161px]';
const widthClass = isPreview ? 'w-[102px]' : isNetflixOriginal ? 'w-[154.04px]' : 'w-[103px]';
const gapClass = isNetflixOriginal ? 'gap-[6.44px]' : 'gap-[7px]';
const paddingClass = isNetflixOriginal ? 'pb-10' : '';
const roundedClass = isPreview ? 'rounded-full' : 'rounded-xs';

const sizes = isPreview ? '102px' : isNetflixOriginal ? '154.04px' : '103px';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previews와 다른 movie list가 하나의 product list에서 처리되는데요, 동일하게 카테고리가 상단에 적혀 있고, 가로로 스크롤하는 형태이지만, 이 둘의 모양이 명백하게 다르기 때문에 언젠가 previews의 모양만 바꾸게 되는 경우에도 movie list가 포함된 product list 컴포넌트를 수정해야 한다는 점에 대해 생각해 보시면 좋을 것 같습니다.

저희가 컴포넌트를 사용하는 것은 재사용성을 높이고 유지보수하기 편리한 형태를 만들기 위함이지만, 재사용성을 높이는 것과 유지보수가 항상 함께 가는 것은 아니니까요! 이런 많고 복잡한 상태 분기가 오히려 유지보수를 어렵게 할 수 있습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 말씀 감사합니다! 해당 부분은 충분히 고려해서 더 좋은 퀄리티의 결과물 만들어낼 수 있도록 하겠습니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클라이언트에서 직접 tmdb에 fetch 요청을 날리는 대신 넥스트 자체 서버를 거쳐 가도록 설정하신 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

백엔드에서 API 호출할 때 저런 식으로 사용을 하는 거로 알고 있어서, 연습도 할 겸 저렇게 코딩했습니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

과제인 만큼 api route를 경험해본 건 좋은 전략 같네요! 실제 프로젝트에 들어가기 전에, 한 번쯤 api route가 필요한 때가 언제일지에 대해서 고민해 보시면 좋을 것 같습니다.

저는 다른 프로젝트에서 바로 백엔드 서버에 fetch를 날리다가 보안 상의 이유로 api route를 사용하게 되었는데요, 클라이언트 -> 백엔드에서 클라이언트 -> 넥스트 서버 -> 백엔드로 하나의 경로가 추가되다 보니 조금 느려지는 감이 있더라고요. 이런 비용을 감수하고도 api route가 어떤 이득을 가져다 주는지 파악하셔서 상황에 맞게 잘 활용하시면 좋겠습니다!

Comment on lines +37 to +41
<Link
key={`${item.type}-${item.id}`}
href={`/${item.type}/${item.id}`}
className={`relative ${heightClass} ${widthClass} flex-shrink-0 transition-transform duration-200 hover:scale-105`}
>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hover 시 살짝 커진 요소가 스크롤 영역 height를 넘어가면서 잘리고, 오히려 세로 스크롤이 되어버리는 버그를 발견했습니다. 스크롤 영역 내에 padding을 설정하거나, 이미지만 커지는 방식으로 수정되면 좋을 것 같아요.

hover 시 scale을 조정하는 스타일을 많이 사용하시는 것 같은데 앞으로도 이런 부분 잘 체크하시면 완성도가 더 높아질 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다! 저 부분은 그냥 대충 넘겼어서 깊게 고려를 못 했던 것 같네요 ㅜㅜ

Comment on lines +8 to +25
/* Headline Typegraphy System */
--text-headline-01: 26.75px;
--text-headline-01--line-height: 20px;
--text-headline-01--letter-spacing: -0.07px;
--text-headline-01--font-weight: 700;

--text-headline-02: 20.92px;
--text-headline-02--line-height: 15.64px;
--text-headline-02--letter-spacing: -0.06px;
--text-headline-02--font-weight: 700;

/* Subhead Typegraphy System */
--text-subhead-01: 20.46px;
--text-subhead-01--line-height: 30px;
--text-subhead-01--letter-spacing: -0.06px;
--text-subhead-01--font-weight: 600;

--text-subhead-02: 13.72px;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런식으로 타이포 설정이 가능했군요... 여태 타이포 관련 유틸리티 클래스를 별도로 작성하는 방식으로 작업했는데 좋은 방법 알아갑니다

Copy link

@seoyeon5117 seoyeon5117 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른분들이 리뷰를 잘해주셔서 리뷰할게 별로 없네요,,, 6주차 과제하시느라 수고하셨습니다!

Comment on lines +12 to +14
const handleComplete = () => {
router.push('/home');
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드가 한 줄이라 화살표 함수로 작성하면 깔끔할 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다!

Comment on lines +7 to +9
<span className="text-caption-01 text-grayscale-02-white">TV Shows</span>
<span className="text-caption-01 text-grayscale-02-white">Movies</span>
<span className="text-caption-01 text-grayscale-02-white">My List</span>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자식 요소에서 반복해서 같은 폰트를 사용하니 부모 컴포넌트에서 한번만 명시해줘도 좋을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안 그래도 이 부분 고민중이었는데 참고하겠습니다 감사합니다!

Comment on lines +23 to +27
const heightClass = isPreview ? 'h-[102px]' : isNetflixOriginal ? 'h-[251px]' : 'h-[161px]';
const widthClass = isPreview ? 'w-[102px]' : isNetflixOriginal ? 'w-[154.04px]' : 'w-[103px]';
const gapClass = isNetflixOriginal ? 'gap-[6.44px]' : 'gap-[7px]';
const paddingClass = isNetflixOriginal ? 'pb-10' : '';
const roundedClass = isPreview ? 'rounded-full' : 'rounded-xs';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clsx utils를 활용하셔도 좋을 것 같습니다!

Comment on lines +11 to +14
option?: string;
}

export default function ProductList({ title, path, option }: ProductListProps) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

option이 필수 요소가 아니니까 default 값을 주는 것은 어떨까요? option = ''와 같은 식으로요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 option이 들어오지 않을 경우 자동으로 undefined되어서 다음과 같이 처리했습니다!

Comment on lines +40 to +43
<div className="border-grayscale-02-white flex h-[15px] w-[15px] flex-col items-center -space-y-1 border-1 border-solid">
<span className="text-caption-04 text-grayscale-02-white">TOP</span>
<span className="text-caption-03 text-grayscale-02-white">10</span>
</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 아이콘 사용 안하시고 직접 구현하셨네요 👀

return (
<div>
{Array.from({ length: 7 }).map((_, i) => (
<div key={i} className="bg-background-02 mb-1 flex w-full animate-pulse items-center">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배경은 그대로 두고 안에 있는 요소들을 깜빡거리게 만드는 식이면 예쁠 것 같아요! 그리고 animate-pulse가 살짝 빠르고 보통의 스켈레톤 컴포넌트들이 깜빡일 때의 색 변화에 비해 더 어두워지는 감이 있어 애니메이션을 직접 구현하셔도 좋을 것 같다는 생각이 듭니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 방법인 것 같습니다! 참고해서 다음 과제에 적용해 보겠습니당

Comment on lines +21 to +24
const { data } = useSuspenseQuery<ThumbnailData>({
queryKey: ['tmdb', path],
queryFn: async () => (await fetch(`${process.env.NEXT_PUBLIC_BASE_URL}${path}`)).json(),
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저희는 useQuery 썼는데 useSuspenseQuery도 있군요... 배우고 갑니다~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useSuspenseQuery는 클라이언트-사이드에서만 구동해서 prefetchQuery랑 같이 사용해야 하니 이런 부분 참고하시면 좋을 거 같아요!

Comment on lines +26 to +28
<Link key={`${product.type}-${product.id}`} href={`/${product.type}/${product.id}`}>
<PlayIcon className="hover:text-background-03-hr transition-color duration-200" />
</Link>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onClick과 navigate로 구현해도 좋지 않을까 싶습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

공식 문서에서 next/link 를 추천하고 있는데 한 번 보시면 좋을 거 같아요!

https://nextjs.org/docs/pages/api-reference/components/link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아앗 navigate 말고 router.push()요! next.router에서 import 해오는거니까 괜찮지 않을까 했습니다. Link는 뭔가 a 태그 같은 느낌이라 불필요하게 태그로 감싸줄 필요가 없다고 생각돼서요!

Comment on lines +24 to +27
"@app/*": ["src/app/*"],
"@components/*": ["src/components/*"],
"@models/*": ["src/models/*"],
"@services/*": ["src/services/*"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public은 public/이라 따로 명시를 해줘야하지만 나머지는 모두 src 폴더 아래에 있어서 "@/": ["src/*"]로 쓰고 "@/app/~"이런식으로 쓰는 것도 편할 것 같습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants