Skip to content
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

[6주차] Team BeatBuddy 김동혁 & 김수현 미션 제출합니다. #14

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

Conversation

Shunamo
Copy link

@Shunamo Shunamo commented May 17, 2024

🔗 링크

배포링크](https://next-netflix-19th-nu.vercel.app/)

📄 Key Questions

1. 정적 라우팅(Static Routing)/동적 라우팅(Dynamic Routing)이란?

정적 라우팅
= 고정된 페이지로 라우팅 하는 것

경로가 buildtime 에 결정된다. 각 페이지에 대한 라우트가 사전에 정의되어 있으며, 사용자가 해당 페이지를 요청하면 라우팅이 발생한다. 콘텐츠가 자주 변경되지 않는 경우에 적합한 방법이다.

동적 라우팅
= 가변적인 페이지
로 라우팅 하는 것

경로가 runtime 에 결정된다. url의 일부분이 동적 매개변수로 처리되며, 이를 기반으로 페이지가 랜더링된다. 글의 ID에 따라 페이지 내용이 달라지는 경우에 적합한 방법이다.

동적 라우팅은 동적 데이터에서 경로를 생성할 때 사용된다. 경로 세그먼트 이름을 미리 알 수 없을 때, 동적 세그먼트를 사용하여 요청 시점에 경로를 생성하거나 빌드 시점에 미리 렌더링할 수 있다.

이번 미션에서 사용한 동적 라우팅 파일구조

image

app / detail / [id] / page.tsx

detail 폴더 내에 [id] 폴더가 있고 [id]폴더 내에 page.tsx 파일이 있다.
여기서 [id]는 동적 매개변수로, URL의 일부가 변할 수 있음을 나타낸다. 예를 들어, 사용자가 /detail/123을 요청하면 [id]123이 된다. 그리고 fetchDiscoverMovie 함수가 호출되어 ID가 123인 영화의 데이터를 가져오고, 그 데이터를 기반으로 페이지가 렌더링된다.


2.성능 최적화를 위해 사용한 방법

Footer.tsx

  1. useEffect 통합

전 : 두 개의 useEffect 훅이 각각 pathnameactiveImage 상태를 업데이트하고 관리

useEffect(() => {
const currentPath = pathname.split('/').pop();
if (currentPath) {
setActiveImage(currentPath);
}
}, [pathname]);

useEffect(() => {
if (activeImage && !pathname.endsWith(/${activeImage})) {
router.push(/${activeImage});
}
}, [activeImage, pathname, router]);

후: 하나의 useEffect 훅으로 통합

useEffect(() => {
  const currentPath = pathname.split('/').pop();
  if (currentPath && currentPath !== activeImage) {
    setActiveImage(currentPath);
  }
}, [pathname, activeImage]);

2. useCallback 사용

: handleImageClickgetImageSrc 함수가 매번 재정의 됨

const handleImageClick = (image: string) => {
if (activeImage !== image) {
setActiveImage(image);
}
};

const getImageSrc = (image: string) => {
return activeImage === image ? /${image}-active.svg : /${image}.svg;
};

: useCallback을 사용하여 함수가 불필요하게 재정의되지 않도록 최적화

const handleImageClick = useCallback(
(image: string) => {
if (activeImage !== image) {
setActiveImage(image);
router.push(/${image});
}
},
[activeImage, router]
);

const getImageSrc = useCallback(
(image: string) => (activeImage === image ? /${image}-active.svg : /${image}.svg),
[activeImage]
);

3. 메뉴 아이템 배열화

: 각 메뉴 아이템을 개별적으로 정의하여 중복 코드가 많았음

image

: 메뉴 아이템을 배열로 관리하여 중복 코드를 줄이고 가독성을 높임

const menuItems = [
{ name: 'main', label: 'Home', width: 24, height: 24 },
{ name: 'search', label: 'Search', width: 24, height: 24 },
{ name: 'comingsoon', label: 'Coming Soon', width: 20, height: 20 },
{ name: 'download', label: 'Download', width: 16, height: 19 },
{ name: 'more', label: 'More', width: 21, height: 17 },
];

{menuItems.map((item) => (
<div key={[item.name](http://item.name/)} className="flex flex-col items-center justify-between h-[35px]">
<Image
src={getImageSrc([item.name](http://item.name/))}
alt={item.label}
width={item.width}
height={item.height}
onClick={() => handleImageClick([item.name](http://item.name/))}
/>
<span className="text-white" style={{ fontSize: '8.2px' }}>
{item.label}
</span>
</div>
))}

3. 전반적인 협업 과정

파트를 나누고 각자가 하고 싶은 파트를 선택했습니다. 필요한 부분은 서로에게 부탁하여 협업했습니다. 과제를 언제 시작하고 마무리할지에 대해 논의하고, 파트너를 믿고 맡겼습니다.
모르는 내용이 있을 때 파트너에게 물어보면, 척척 알려주고 방법까지 상세히 설명해줘서 도움을 많이 받았습니다.


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.

너무 멋진 애니메이션에서 많이 배울 수 있었던 것 같습니다.
framer-motion도 너무 잘 쓰시고 코드도 전반적으로 정말 깔끔한 것 같아요!

다만 코드의 인덴테이션 스타일과 api token 노출에 대한 부분 조금만 더 신경써 주시면 정말 좋을 것 같아요.

이번 과제도 너무 수고 많으셨어요 🙇‍♂️


export const fetchDiscoverMovie = async (endpoint: string) => {
const url = `${process.env.NEXT_PUBLIC_URL}${endpoint}`;
console.log("Requesting URL:", url); // URL 로깅

Choose a reason for hiding this comment

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

배포 시에 불필요한 콘솔 출력문은 잊지 않고 지워주시면 좋을 것 같습니다 💪🏼

});

if (!res.ok) {
throw new Error(`API call failed with status: ${res.status}`);

Choose a reason for hiding this comment

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

스크린샷 2024-05-19 오전 2 06 01

확실하지는 않은데, 브라우저의 네트워크 탭을 까보면 BeatBuddy 팀원 분들께서 환경변수 처리되는 api token이 노출되고 있는 것 같아요.

nextJS의 rewrites 속성을 통해 api key를 숨기는 방법에 대해서 고민해보셔도 좋을 것 같습니다.

Copy link

Choose a reason for hiding this comment

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

오 저도 배워갑니다 !

};
};

export default async function Page({ params }: MovieDetailProps) {

Choose a reason for hiding this comment

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

서버 컴포넌트에서 동적으로 변하는 path를 params 속성을 구조 분해 방식으로 받는 것을 잘 이해하고 계신 것 같습니다 👍

Comment on lines 17 to 32
useEffect(() => {
if (typeof window !== 'undefined' && !audio) {
setAudio(new Audio('/NetflixIntroSound.mp3'));
}
}, [audio]);

useEffect(() => {
if (play && audio) {
audio
.play()
.then(() => {
setAudioAndAnimationFinished(true);
})
.catch((error: Error) => console.error('Playback failed:', error));
}
}, [play, audio]);

Choose a reason for hiding this comment

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

BeatBuddy 팀원 분들은 멋진 애니메이션 효과와 UX에 정말 진심이신 것 같아요...!

훅의 실행 시점 조절을 통해 오디오 상태를 띄우고 실행시키는 로직을 참 잘 짜시는 것 같습니다. 각각의 실행 로직이 다음 화면 전환으로 너무 잘 이해가 가는 코드인 것 같구요 👍

Comment on lines 20 to 26
const fetchPopular = await fetchDiscoverMovie(apiEndpoints.popularmovies);
const fetchTrend = await fetchDiscoverMovie(apiEndpoints.trendingmovies);
const fetchTopRated = await fetchDiscoverMovie(apiEndpoints.topratedmovies);
const fetchOnAir = await fetchDiscoverMovie(apiEndpoints.ontheairmovies);
const fetchNowPlaying = await fetchDiscoverMovie(apiEndpoints.nowplayingmovies);
const fetchUpComing = await fetchDiscoverMovie(apiEndpoints.upcomingmovies);
const fetchTopRated1 = await fetchDiscoverMovie(apiEndpoints.topRatedMovies)

Choose a reason for hiding this comment

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

다양한 data를 fetch해오는 로직을 promise.all() 메서드를 이용해 동일 시점에 비동기 로직이 끝남을 보장 받아도 좋을 것 같습니다!

Comment on lines +1 to +6
'use client';
import Image from 'next/image';
import { useState, useEffect, useCallback } from 'react';
import { useRouter, usePathname } from 'next/navigation';

export default function Footer() {

Choose a reason for hiding this comment

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

공통적으로 많이 나나는 Footer 컴포넌트를 랜딩 페이지를 제외한 레이아웃을 group route로 묶어서 만들어주시는 것이 렌더링 성능에 있어서 더 좋다고 생각합니다.

layout.tsx는 리렌더링이 되지 않아 불필요한 오버헤드를 줄일 수 있다고 해요!

Comment on lines +16 to +21
<div className="flex flex-col h-screen">
<div className="flex-1 overflow-auto scrollbar-hide">
<MovieDetail movie={movie} />
</div>
<Footer/>
</div>);

Choose a reason for hiding this comment

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

전체적으로 프로젝트 코드를 보면서 느낀 것인데, JSX element의 code indentation이 prettier 설정의 문제인 건지 제대로 지켜지지 않고 있는 것 같아요!

이를 수정하셔서 가독성을 더 높이시면 좋을 것 같습니다.

Copy link

Choose a reason for hiding this comment

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

그러게요! .prettierrc 파일은 잘있는데 extension이 없는 문제일수도 있고 아니면 system setting에서 수동으로 해줘도 되긴 해요 !!

<div className="text-white">
<div className="relative w-full h-[415px] overflow-hidden mb-4">
<Image
src={`https://image.tmdb.org/t/p/w500${movie.poster_path}`}

Choose a reason for hiding this comment

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

Suggested change
src={`https://image.tmdb.org/t/p/w500${movie.poster_path}`}
src={`https://image.tmdb.org/t/p/w1280${movie.poster_path}`}

약간 렌더링 되는 이미지의 해상도가 살짝 아쉬운데, 위와 같이 너비를 다르게 해서 요청을 날려보시면 어떨까 싶어요!

Comment on lines +22 to +24
style={{ objectFit: 'cover' }}
alt={movie.title}
className="rounded-md"

Choose a reason for hiding this comment

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

Suggested change
style={{ objectFit: 'cover' }}
alt={movie.title}
className="rounded-md"
alt={movie.title}
className="rounded-md object-cover"

어차피 tailwindCSS를 사용하시니, 인라인 방식의 스타일링 말고 이렇게 바꿔보시는 게 좋을 것 같아요!

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 +7
/** @type {import('next').NextConfig} */
const nextConfig = {
images: {
domains: ['image.tmdb.org'], // Add other domains as needed
},
};
export default nextConfig;

Choose a reason for hiding this comment

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

아래에서 코멘트 드린 바와 같이, next.config.mjs 설정 파일에서 rewrites() 함수를 이용해 api token의 노출을 막아보시면 좋을 것 같습니다

</button>
</div>
<h1 className="text-[26.75px] font-bold leading-[20px] mb-6">Previews</h1>
<p className="text-[11.14px] text-left">{movie.overview}</p>

Choose a reason for hiding this comment

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

스크린샷 2024-05-19 오전 2 31 30

이것처럼 일부 fetching 된 data에는 preview 설명글이 존재하지 않는 경우도 있는 것 같아요! 이런 부분은 조건부 검사하셔서 대체 텍스트 보여주셔도 좋을 것 같아요

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

@songess songess left a comment

Choose a reason for hiding this comment

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

6주차 과제 고생하셨습니다!

framer-motion을 통한 애니메이션 덕분에 재밌게 리뷰할 수 있었어요! 승완이형이 말해주신 것처럼 API요청시 개발자도구 탭에서 토큰이 노출되는 부분을 개선할 수 있으면 더 좋을 거 같아요!

return (
<div className="flex justify-center items-center h-screen bg-gradient-to-black ">
<div className="flex justify-center items-center ">
<NetflixButton />
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 +34 to +44
<Poster title="Popular on Netflix" fetchData={fetchPopular} />
<Poster title="Trending Now" fetchData={fetchTrend} />
<Poster title="Top 10 in Nigeria Today" fetchData={fetchTopRated} />
<Poster title="Airing Today" fetchData={fetchOnAir} />
<Poster title="Now Playing" fetchData={fetchNowPlaying} />
<Poster title="Upcoming" fetchData={fetchUpComing} />
<BigPoster title="Netflix Originals" fetchData={fetchPopular} />
<Poster title="Popular on Netflix" fetchData={fetchPopular} />
<Poster title="Trending Now" fetchData={fetchTrend} />
<Poster title="Top 10 in Nigeria Today" fetchData={fetchTopRated} />
<Poster title="Airing Today" fetchData={fetchOnAir} />
Copy link

@songess songess May 19, 2024

Choose a reason for hiding this comment

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

저도 똑같은 생각입니다!

...
const arr = [
    { title: "Popular on Netflix", fetchData: fetchPopular },
    { title: "Trending Now", fetchData: fetchTrend },
    { title: "Top 10 in Nigeria Today", fetchData: fetchTopRated },
    { title: "Airing Today", fetchData: fetchOnAir },
    { title: "Now Playing", fetchData: fetchNowPlaying },
    { title: "Upcoming", fetchData: fetchUpComing },
    { title: "Netflix Originals", fetchData: fetchPopular },
    { title: "Popular on Netflix", fetchData: fetchPopular },
    { title: "Trending Now", fetchData: fetchTrend },
    { title: "Top 10 in Nigeria Today", fetchData: fetchTopRated },
    { title: "Airing Today", fetchData: fetchOnAir },
  ]
...
      arr.map((item, index) => {
        if (index === 6) {
          return <BigPoster title={item.title} fetchData={item.fetchData} />
        } else {
          return <Poster title={item.title} fetchData={item.fetchData} />
        }
      })}
...

이런식으로요!

return (
<div className="flex flex-col h-screen">
<div className="flex-1 overflow-auto scrollbar-hide">
<Search fetchData={fetchTopRated} title="Top Rated Movies" />
Copy link

Choose a reason for hiding this comment

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

Input창은 따로 컴포넌트를 분리시켜 상단에 고정되어있어도 좋을 거 같아요!

const loadMore = async () => {
if (isLoading || !hasMore) return;
setIsLoading(true);
const newPage = page + 1;
Copy link

Choose a reason for hiding this comment

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

새로운 변수를 선언해주지 않고 page를 처음부터 2로 설정하고 API요청을 하고 1 증가시키는 방식으로 해도 좋을 거 같아요!

if (inputValue === '') {
setFilteredData(data);
} else {
setFilteredData(data.filter((movie) => movie.title.toLowerCase().includes(inputValue.toLowerCase())));
Copy link

Choose a reason for hiding this comment

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

대소문자 모두 검색되게 한점 멋져요!

/>
</motion.div>
<p className="text-white">로딩중 ~</p>
</motion.div>
Copy link

Choose a reason for hiding this comment

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

애니메이션 기능 멋져요!

<div
className="w-full flex pt-1 cursor-pointer"
key={movie.id}
ref={index === filteredData.length - 1 ? lastElementRef : null}>
Copy link

Choose a reason for hiding this comment

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

ref를 마지막 요소에 찍게 해서 필요없는 div태그가 생기지 않게한 부분 멋져요!

if (observer.current) observer.current.disconnect();
observer.current = new IntersectionObserver(
(entries) => {
if (entries[0].isIntersecting) {
Copy link

Choose a reason for hiding this comment

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

현재 검색을 하면 마지막 영화카드에 ref가 들어가 화면에 노출되고 loadMore()가 실행되서 검색어에 따른 영화리스트가 뷰를 전부 채우지 못하면 계속해서 API 호출이 일어나고 있어요. 이 부분이 의도된 것이라면 상관없지만 hasMore상태를 통해 결과값이 하나도 없을때는 함수실행이 안되게 해주신걸로 보아, 필요없는 호출이라고 생각이 들어요. input창에 인풋값이 존재할때 loadMore()가 실행되지 않는 방식으로 수정해도 좋을 거 같아요!

Copy link

@Rose-my Rose-my 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 +16 to +21
<div className="flex flex-col h-screen">
<div className="flex-1 overflow-auto scrollbar-hide">
<MovieDetail movie={movie} />
</div>
<Footer/>
</div>);
Copy link

Choose a reason for hiding this comment

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

그러게요! .prettierrc 파일은 잘있는데 extension이 없는 문제일수도 있고 아니면 system setting에서 수동으로 해줘도 되긴 해요 !!

});

if (!res.ok) {
throw new Error(`API call failed with status: ${res.status}`);
Copy link

Choose a reason for hiding this comment

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

오 저도 배워갑니다 !

Copy link

@wokbjso wokbjso left a comment

Choose a reason for hiding this comment

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

안녕하세요 19기 프론트 멘토 김현민 이라고 합니다~!!

무한 스크롤 할 때마다 예쁜 로딩 상태까지 구현해주셔서 고생이 많으셨겠더라고요 ㅋㅋㅋㅎ
프론트에 진심이신 것 같아 저도 자극 받고 가는 것 같습니다.

이번 과제 너무 고생 많으셨어요~!!!

Comment on lines +17 to +21
const pretendard = localFont({
src: '../lib/fonts/PretendardVariable.woff2',
display: 'swap',
weight: '45 920',
variable: '--font-pretendard',
Copy link

Choose a reason for hiding this comment

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

next/font 사용하신 점 너무 좋습니다 ㅎㅎ

Comment on lines +10 to +14
export const metadata: Metadata = {
title: 'Netflix',
description: 'Netflix clone coding',
icons: {
icon: '/logo.png',
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 19 to 26
export default async function Home() {
const fetchPopular = await fetchDiscoverMovie(apiEndpoints.popularmovies);
const fetchTrend = await fetchDiscoverMovie(apiEndpoints.trendingmovies);
const fetchTopRated = await fetchDiscoverMovie(apiEndpoints.topratedmovies);
const fetchOnAir = await fetchDiscoverMovie(apiEndpoints.ontheairmovies);
const fetchNowPlaying = await fetchDiscoverMovie(apiEndpoints.nowplayingmovies);
const fetchUpComing = await fetchDiscoverMovie(apiEndpoints.upcomingmovies);
const fetchTopRated1 = await fetchDiscoverMovie(apiEndpoints.topRatedMovies)
Copy link

Choose a reason for hiding this comment

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

지금은 상관 없지만 모든 api를 순차적으로 호출하기때문에 network-waterfall 이 발생하네요!

예를 들어 "우선 윗 포스터들에 관한 api를 호출하고, Now Playing 포스터가 보이는 순간 나머지 api를 호출" 이런식으로 react-intersection-observer 라이브러리를 사용해서 구현해보는 것도 나쁘지 않을 것 같아요 ㅎㅎ

alt={movie.title}
className="rounded-md"
/>
<div className="absolute inset-0 bg-gradient-to-b from-black via-transparent to-black"></div>
Copy link

Choose a reason for hiding this comment

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

요 스타일 컴포넌트로 생성해 주시는게 가독성에도 좋고 MovieDetail.tsx 에서도 같은 코드가 사용되고 있어 재사용성에도 좋을 것 같아요!!

Suggested change
<div className="absolute inset-0 bg-gradient-to-b from-black via-transparent to-black"></div>
<GradientStyle />

<div className='flex justify-center'>
<div className="flex items-center justify-between mt-2.5 mb-10" style={{ width: '259px', height: '45px' }}>
<button className="flex flex-col items-center justify-center">
<Image src="/plus-icon.svg" alt="Add to My List" width={24} height={24} />
Copy link

Choose a reason for hiding this comment

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

아이콘은 굳이 Image 태그를 사용하지 않아도 될 것 같아요!!

svg 를 그대로 import 해와서 컴포넌트처럼 사용할 수 있는데 해당 방법에 대해 찾아보시면 좋을 것 같네요~~

Comment on lines +12 to +17
backgroundImage: {
'gradient-radial': 'radial-gradient(var(--tw-gradient-stops))',
'gradient-conic': 'conic-gradient(from 180deg at 50% 50%, var(--tw-gradient-stops))',
'gradient-to-b': 'linear-gradient(to bottom, rgba(0, 0, 0, 0.45), rgba(0, 0, 0, 0) , rgba(0, 0, 0, 1))',
'gradient-to-black': 'radial-gradient(circle, rgba(28,28,28,1) 0%, rgba(0,0,0,1) 100%)',
},
Copy link

Choose a reason for hiding this comment

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

다소 복잡한 CSS 스타일을 보기 편한 네이밍으로 바꿔두신 점 너무 좋네요 ㅎㅎㅎ

Comment on lines +20 to +22
export interface PreviewrProps {
fetchData: FetchData;
}
Copy link

Choose a reason for hiding this comment

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

보니까 PreviewProps 에서는 id, title, poster_path 이정도만 사용되는 것 같더라고요??

FetchData 인터페이스를 모두 부여할 필요 없이 유틸리티 타입을 활용해서 유연하게 대처할 수 있을 것 같아요!!
(https://kyounghwan01.github.io/blog/TS/fundamentals/utility-types/)

Suggested change
export interface PreviewrProps {
fetchData: FetchData;
}
interface MovieInfo{
id: number;
backdrop_path: string;
poster_path: string;
title: string;
overview: string;
adult: boolean;
original_language: string;
original_title: string;
popularity: number;
release_date: string;
video: boolean;
vote_average: number;
vote_count: number;
}
export interface FetchData{
page:number;
results: MovieInfo[];
}
export interface PreviewrProps{
fetchData: Pick<MovieInfo, 'id' | 'title' | 'poster_path'>[]
}

Comment on lines 13 to 35
<header className="flex items-center justify-center h-16 px-6 bg-transparent text-white mt-4 mr-2 absolute top-0 left-0 right-0 z-10">
<div className="flex items-center justify-between w-full max-w-screen-lg">
<Image src="/logos_netflix-icon.svg" alt="Logo" width={56.67} height={57} />
<span
className={activeMenu === 'tvshows' ? 'text-white font-semibold' : 'text-white hover:text-white cursor-pointer'}
onClick={() => handleMenuClick('tvshows')}
>
TV Shows
</span>
<span
className={activeMenu === 'movies' ? 'text-white font-semibold' : 'text-white hover:text-white cursor-pointer'}
onClick={() => handleMenuClick('movies')}
>
Movies
</span>
<span
className={activeMenu === 'mylist' ? 'text-white font-semibold' : 'text-white hover:text-white cursor-pointer'}
onClick={() => handleMenuClick('mylist')}
>
My List
</span>
</div>
</header>
Copy link

Choose a reason for hiding this comment

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

요 부분도 각 클릭할 수 있는 부분들을 컴포넌트로 생성해 주면 가독성에 더 좋을 것 같아요!!

Suggested change
<header className="flex items-center justify-center h-16 px-6 bg-transparent text-white mt-4 mr-2 absolute top-0 left-0 right-0 z-10">
<div className="flex items-center justify-between w-full max-w-screen-lg">
<Image src="/logos_netflix-icon.svg" alt="Logo" width={56.67} height={57} />
<span
className={activeMenu === 'tvshows' ? 'text-white font-semibold' : 'text-white hover:text-white cursor-pointer'}
onClick={() => handleMenuClick('tvshows')}
>
TV Shows
</span>
<span
className={activeMenu === 'movies' ? 'text-white font-semibold' : 'text-white hover:text-white cursor-pointer'}
onClick={() => handleMenuClick('movies')}
>
Movies
</span>
<span
className={activeMenu === 'mylist' ? 'text-white font-semibold' : 'text-white hover:text-white cursor-pointer'}
onClick={() => handleMenuClick('mylist')}
>
My List
</span>
</div>
</header>
const headerButtonList=["TV Shows", "Movies", "My List"];
<header className="flex items-center justify-center h-16 px-6 bg-transparent text-white mt-4 mr-2 absolute top-0 left-0 right-0 z-10">
<div className="flex items-center justify-between w-full max-w-screen-lg">
<Image src="/logos_netflix-icon.svg" alt="Logo" width={56.67} height={57} />
{headerButtonList.map(li=>(
<HeaderBtn key={li} text={li} activeMenu={activeMenu} onClick={handleMenuClick} />
))}
</div>
</header>

HeaderBtn 컴포넌트 ->

  return <span className={activeMenu === text ? 'text-white font-semibold' : 'text-white hover:text-white cursor-pointer'}
          onClick={() => handleMenuClick(text)}
        >
          {text}
        </span>

Comment on lines +117 to +128
{isLoading && (
<motion.div
className="flex justify-center items-center flex-col py-5"
initial={{ opacity: 0 }}
animate={{ opacity: 1 }}
exit={{ opacity: 0 }}>
<motion.div
className="flex justify-center py-5"
initial={{ opacity: 0 }}
animate={{ opacity: 1 }}
exit={{ opacity: 0 }}>
<motion.div
Copy link

Choose a reason for hiding this comment

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

오 framer-motion 사용 해서 로딩 애니메이션을 구현하셨네요!!! 저도 framer-motion 즐겨쓰는 편인데 멋지네요 ㅎㅎ

Comment on lines 12 to 13
return (
<header className="flex items-center justify-center h-16 px-6 bg-transparent text-white mt-4 mr-2 absolute top-0 left-0 right-0 z-10">
Copy link

Choose a reason for hiding this comment

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

스크롤을 내릴 때 헤더에 background를 설정해준다면 스크롤 해도 헤더가 잘 보일 것 같아요 ㅎㅎ

Copy link

@noeyeyh noeyeyh left a comment

Choose a reason for hiding this comment

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

BeatBuddy팀 안녕하세요! 저번 과제에 이어 코드리뷰 남기고 갑니다..
항상 css에 진심이셔서 코드 보는 재미가 있어요!

다음 과제도 화이팅해요!!

<div className="text-[20.9px] font-bold text-white">{title}</div>
<div className="flex overflow-x-auto pt-1 gap-x-2 scrollbar-hide">
{fetchData.results?.map((movie, index) => (
<div key={index} className="inline-block">
Copy link

Choose a reason for hiding this comment

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

배열을 렌더링할 때, 각 요소의 키 값을 배열의 인덱스로 사용하는 것은 지양해야 한다고 합니다. 대신에 movie.id를 key값으로 지정해줘도 좋을 것 같아요.

[React] 배열의 index를 key로 쓰면 안되는 이유

display: 'none', // Chrome, Safari, Opera
},
},
};
Copy link

Choose a reason for hiding this comment

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

plugin 함수를 통해 스크롤바를 숨기셨네요! 디테일..


export interface TopRatedProps {
fetchData: FetchData;
}
Copy link

Choose a reason for hiding this comment

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

중복을 줄이기 위해, 공통된 프로퍼티를 가지는 인터페이스는 하나로 통합해서 사용하면 더 좋을 것 같아요!

<Link href={`/detail/${movie.id}`} key={movie.id}>
<div
className="w-full flex pt-1 cursor-pointer"
key={movie.id}
Copy link

Choose a reason for hiding this comment

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

Link랑 내부의 div에 key 속성이 중복으로 사용되고 있어요. Link 에만 key를 설정하면 될 것 같아요!

</Link>
<div className="absolute bottom-0 left-0 right-0">
<div className="flex items-center justify-center space-x-2">
<Image src="/Top10Icon.svg" alt="Top 10 Icon" width={15} height={15} />
Copy link

Choose a reason for hiding this comment

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

저희팀은 이번 과제에서 SVG 이미지는 컴포넌트화해서 사용했어요! 이미 최적화된 파일이니 굳이 Image 컴포넌트를 사용하지 않아도 될 것 같아요.

@Shunamo Shunamo changed the title Team BeatBuddy 김동혁 & 김수현 미션 제출합니다. [6주차] Team BeatBuddy 김동혁 & 김수현 미션 제출합니다. Jun 23, 2024
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.

7 participants