-
Notifications
You must be signed in to change notification settings - Fork 14
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주차] gotcha팀 미션 제출합니다 #14
base: master
Are you sure you want to change the base?
Conversation
Feature/1 setting
[chore] 라우팅수정
Feature/3 mainpage
Feature/1 setting/ api연동
style : MainControlBox style
[chore] 브랜치충돌 해결
style : MainImage style
Feature/4 movie list
[feat]화면 반응형으로 조정
Feature/7 api connect re
Feature/6 style change
Feature/8 detail
[feat] 검색 기능 구현
Feature/11 detail check
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.
이번 한 주 동안 고생 많으셨습니다~
리액트 쿼리를 도입하려고 하신게 특히 인상 깊었습니다!! 일요일날 봬요 :)
baseURL: BASE_URL, | ||
headers: { | ||
"Content-Type": "application/json", | ||
Authorization: `Bearer ` + process.env.NEXT_PUBLIC_AUTH, |
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.
.env 파일 숨기신 점 잘하신 거 같아요:)
return ( | ||
<div | ||
className={`fixed top-0 flex justify-between items-center w-full max-w-[450px] h-[57px] px-4 md:px-5 lg:px-6 py-8 pr-8 z-50 transition-all duration-300 ease ${ | ||
scrollPosition > 100 |
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
안에서 scrollPosition에 따라 배경을 다르게 하신 디테일 너무 좋아요~~
@@ -0,0 +1,93 @@ | |||
'use client'; |
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.
만약에 SSR로 구현한다면, 상세페이지에서 useEffect
를 쓰는 대신에 async() - await 패턴을 적용해서 data fetching을 진행할 수 있을 거 같습니다!
https://nextjs.org/docs/app/building-your-application/data-fetching/fetching-caching-and-revalidating
"use client"; | ||
|
||
import React, { useState } from "react"; | ||
import { Hydrate, QueryClientProvider, QueryClient } from "react-query"; |
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 ( | ||
<div className="flex flex-col w-full h-screen items-start max-w-[450px] scrollbar-none"> |
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.
지금은 검색했을 때 이미지가 렌더링되면 검색 부분 height가 밑에서 지정해준 20으로 줄어들고 잇는데, 검색 부분의 height
특정 값으로 지정해주면 좋을 것 같아요!
|
||
const MainControlBox = () => { | ||
return ( | ||
<div className="flex flex-col items-center gap-11 pt-2.5 px-11 pb-11 w-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.
Taillwind 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.
tailwind 로 마이그레이션에 일부 성공하셨다는 점에서 박수 드립니다 ㅋㅋㅋㅋㅋ 너무 수고하셨고 작성하신 코드를 읽어보며 많은 공부가 되었습니다. 시간관계상 양질의 코드리뷰를 못해드린점 죄송하고 수고하셨습니다!!
|
||
const api = axios.create({ | ||
baseURL: BASE_URL, | ||
headers: { | ||
"Content-Type": "application/json", | ||
Authorization: `Bearer ` + process.env.NEXT_PUBLIC_AUTH, |
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.
axios 인스턴스 설정 좋네요 ㅎㅎ
<Image | ||
src={'/images/netflix-logo.svg'} | ||
width={57} | ||
height={57} | ||
alt="logo" | ||
/> |
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.
next/image 를 통해 최적화 하신 점 좋네요 ㅎㅎ
const MainControlBox = () => { | ||
return ( | ||
<div className="flex flex-col items-center gap-11 pt-2.5 px-11 pb-11 w-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.
오 tailwind 로 마이그레이션 성공하셨네요 ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 축하드립니다
@@ -0,0 +1,92 @@ | |||
'use client'; |
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.
추후 리팩토링을 통해 페이지 최상단 'use client' 선언을 방지한다면 사용자 경험이 더욱 개선될 것 같아요!!
|
||
const PlayBut = styled.div` | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; | ||
|
||
width: 18.9375rem; | ||
height: 2.8125rem; | ||
border-radius: 0.35156rem; | ||
background: #c4c4c4; |
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.
시간상 모든 부분은 tailwind 로 바꾸지 못하신 것 같네요 ㅋㅋㅋㅋㅋㅋ
|
||
<div className="flex flex-col pl-4 gap-3 pb-8"> | ||
{upComingData && ( | ||
<MainItemList title="Previews" data={upComingData} circle={true} /> | ||
)} | ||
{popularData && ( | ||
<MainItemList | ||
title="Popular on Netflix" | ||
data={popularData} | ||
circle={false} | ||
/> | ||
)} | ||
{topRatedData && ( | ||
<MainItemList | ||
title="Trending Now" | ||
data={topRatedData} | ||
circle={false} | ||
/> | ||
)} | ||
{nowPlayingData && ( | ||
<MainItemList | ||
title="Now Playing" | ||
data={nowPlayingData} | ||
circle={false} | ||
/> | ||
)} | ||
</div> |
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.
이 부분 MainItemList 의 정보를 array 변수로 선언한 뒤 map 함수로 렌더링한다면 가독성과 추후 확장성에 더 좋을 것 같다는 개인적인 의견 드립니다!!
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 ( | ||
<div | ||
className={`fixed top-0 flex justify-between items-center w-full max-w-[450px] h-[57px] px-4 md:px-5 lg:px-6 py-8 pr-8 z-50 transition-all duration-300 ease ${ |
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.
tailwind로 바꾸신 점 좋은 것 같습니다:)
handleItemClick(id, link); | ||
}} | ||
selected={selected} | ||
> |
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.
handleItemClick로 링크를 처리하는 대신 next/Link 이용해서 코드를 작성하면 더 간결해질 수 있을 것 같아요!
{searchResults && | ||
searchResults.results.map((movie: any) => ( | ||
<SearchingItem key={movie.id} movie={movie} /> | ||
))} |
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.
앗ㅜㅜ 해당 항목 디테일을 놓쳤네요. 추후 수정할 수 있도록 하겠습니다!!
fill | ||
style={{ | ||
objectFit: "cover", | ||
}} |
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.
따로 대체 이미지를 넣지는 않고, poster-url이 없을 때는 회색 박스가 뜨도록 설정해놨었어요! 그런데 추후에 스켈레톤 컴포넌트 추가하게 된다면 헷갈릴 수 있으니 대체 이미지를 넣는 방식도 좋을 것 같습니다😊
alt={"delete"} | ||
width={28} | ||
height={28} | ||
/> |
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.
버튼을 눌럿을 때 detail 화면으로 넘어가야 하는데, 넘어가지 않는 것 같아요!
}} | ||
/> | ||
)} | ||
<div className="absolute z-10 w-full h-full bg-gradient-to-b from-black via-transparent to-black"></div> |
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.
gradient 적용하신 디테일 좋은 것 같습니다:)
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.
한 주동안 고생하셨습니다~~ react-query를 사용해서 구현한 점이 인상 깊네요 👍👍
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.
overflow: hidden;
를 이용해서 브라우저의 scroll bar를 없애주면 더 깔끔한 UI가 될거 같아요~!
const { | ||
data: searchResults, | ||
isLoading, | ||
isError, | ||
} = useQuery( | ||
["search", { query: inputValue }], | ||
() => getSearchResults(inputValue), | ||
{ | ||
enabled: inputValue !== "", | ||
} | ||
); |
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.
searchResults
를 가져올 때 아무것도 입력되지 않으면 Top Searches가 뜨지않는 문제가 발생하는거 같아요!
}; | ||
|
||
fetchData(movieId); | ||
console.log(data); |
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.
console.log
는 배포 시 지워주면 좋을거 같아요~~
배포링크
🍿🎞️🍿
Key Question
라우팅은 기본적으로 url이동, 즉 데이터를 보낼 경로를 선택하는 과정으로 리액트는 정적, 동적 라우팅을 지원한다.
정적라우팅은 미리 정해놓은 주소로 이동하는 것으로, 사용자 모두가 같은 페이지로 라우팅 된다.
반면 동적 라우팅은 주소가 자동으로 설정되어 가변적 페이지로 라우팅된다.
이번 과제에서는 상세페이지를 동적라우팅으로 구현했는데 데이터 전달방식에 있어서 queryString이나 pathVariable을 사용하는 것과 props로 전달하는 방식 중 더 효율적인 방식에 대해 고민해 볼 수 있었다.
페이지는 SSR로 구현해도 업데이트가 빈번한 컴포넌트 CSR로 구현해 불필요한 렌더링을 최소화하도록 했다
후기
은비: 이번 넷플릭스 클론은 불러오는 데이터가 많다보니 로딩 속도가 생각보다 길어져서 성능최적화의 필요성을 많이 느낀 미션이었습니당
다들 이번주도 고생하셨어요~~
지혜: 검색 기능 구현하며 react-query를 사용해봤는데, 수많은 에러를 마주했지만... 어찌저찌 해낸 것에 의의를 두려고 합니다. debounce, isLoading 등 더 다양하게 사용해보면 좋았을 것들을 적용해볼 수 있기를 바랍니다... 유독 이번 주차 과제 진행하며 기능 구현 외적인 에러가 많았는데, 이렇게 낼 수 있어서 정말 기쁩니다😂