-
Notifications
You must be signed in to change notification settings - Fork 8
[6주차] Team CatchUp 정성훈&장자윤 과제 제출합니다. #13
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
base: main
Are you sure you want to change the base?
Conversation
react-slick css 라이브러리 추가
const ComingSoonIcon = ({fill="#8C8787", ...props }: IconProps) => (
<svg width="20" height="20" viewBox="0 0 20 20" fill={fill} xmlns="http://www.w3.org/2000/svg" {...props}>
<path d="M2 4H0V18C0 19.1 0.9 20 2 20H16V18H2V4ZM18 0H6C4.9 0 4 0.9 4 2V14C4 15.1 4.9 16 6 16H18C19.1 16 20 15.1 20 14V2C20 0.9 19.1 0 18 0ZM18 14H6V2H18V14ZM10 3.5V12.5L16 8L10 3.5Z" fill={fill}/>
</svg>
)
export default ComingSoonIcon;erge branch 'main' of https://github.com/CEOS-22nd-CatchUP/next-netflix-22nd
|
고생하셨습니다!!각 팀원이 무슨 기능을 위해 어떻게 구현했는지도 작성해주신점도 보기 좋습니다! |
|
전반적으로 파일이나 폴더 수가 엄청 많이 나눠진것같은데 이렇게 하면 협업 과정에서 어려움이 있을 수 있으니 폴더 구조를 조금 더 효율적으로 정리해봐도 좋을것 같습니다 파일도 컴포넌트를 엄청 세분화한것같은데 개인적으로는 조금 더 크게크게 가도 어떨까 싶습니다 |
|
그리고 메인 페이지에서 작품들 옆으로 스크롤해서 보는게 구현되지 않은것같은데 이것도 한번 해보시면 좋을것같습니다!! |
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.
개인적으로 그냥 궁금한 점인데 prettier 코드도 두분이 합의를 보셨나요?? 저희는 안했는데 다른팀은 어떻게 했는지 궁금하네요
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.
이거 svgr 때문에 이렇게 하신거죠 저희도 일단 이렇게 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.
전반적으로 이 components/main 폴더에 있는 파일들은 조금 하나로 합쳐서 조금 더 큰 단위로 컴포넌트 만들어서 작성해도 좋을것같습니다
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.
search 파일들도 조금 더 큰 단위로 묶어도 좋을것같습니다
Tutankhannun
left a comment
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 ( | ||
| <Link href={`/movie/${movie.id}`}> | ||
| <div className="bg-gray-2 flex h-[76px] 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.
Link 태그에도 calssName을 달 수 있어서 불필요한 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.
비슷한 코드가 반복되니 props로 크기/보여줄 개수만 바꿔서 재사용하는건 어떨까요?
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.
현재 마우스로 스크롤할 때 드래그를 클릭으로 인식하여 상세페이지로 넘어가는데 이를 방지하는 state를 만들거나 swiper 라이브러리를 사용해도 좋을 것 같습니다.
dragunshin
left a comment
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.
route segment별로 분리하여 관리한 구조가 인상깊습니다
| <div | ||
| className={clsx('flex flex-col items-center', { | ||
| '-mt-[1.5px]': item.adjustTextTop, | ||
| '-mx-2': item.adjustPx, | ||
| })} | ||
| > | ||
| <div className={clsx('cursor-pointer', { '-mt-px': item.adjustIconTop })}> | ||
| <Icon color={isActive ? '#ffffff' : '#8C8787'} /> | ||
| </div> | ||
| <p className={clsx('cursor-pointer', item.mtClass, { 'text-white': isActive, 'text-gray-1': !isActive })}> | ||
| {item.label} | ||
| </p> | ||
| </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.
버튼 전체를 묶어서 하는 게 클릭 영역이 더 확실히 보장될 것 같아요
| useEffect(() => { | ||
| const idx = BTM_MENU_ITEMS.findIndex((item) => item.path === pathname); | ||
| if (idx >= 0) setActive(idx); | ||
| }, [pathname, setActive]); |
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.
pathname이 자주 바귄다면 useMemo로 최적화 해보는 것도 방법일 것 같아요
| })} | ||
| > | ||
| <div className={clsx('cursor-pointer', { '-mt-px': item.adjustIconTop })}> | ||
| <Icon color={isActive ? '#ffffff' : '#8C8787'} /> |
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를 currentColor로 지정했다면 색상만 바꾸는게 가능할 것 같아요
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를 텍스트로 열어보셨나요?그 방법으로 해결될것 같아요 우클릭 하시고 Open with누르시면 됩니다
hammoooo
left a comment
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.
Zustand로 무한스크롤 구현하신 게 인상적이었습니다! 과제 고생하셨습니다!
| } | ||
|
|
||
| export function CircleSliders({ movies }: SlidersProps) { | ||
| const settings = { |
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.
꽤 간편하게 쓸 수 있는 라이브러리라서 좋은 것 같아요!
| <span>Play</span> | ||
| </div> | ||
| <div className="text-title3-sb mt-8 w-[303px] leading-8">{movie.title}</div> | ||
| <div className="text-text5-sb mt-5 mb-20 w-[303px] text-white/70">{movie.overview}</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.
상세페이지 눌러보다 보니 overview가 없는 것도 있더라고요 조건부 렌더링 처리하면 좋을 것 같습니다!
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.
URL에 API Key를 보내는 방식은 보안적으로 안 좋아서 Authorization 헤더에 보내시는 걸 추천드립니다!
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.
폰트 설정이 디테일하게 되어있는 점 좋습니다!
배포링크
먼저 코드 리뷰에서 이야기가 나온 폴더 구조를 다시 정리했습니다. 그 뒤 각자 맡은 부분을 구현한 뒤, 코드를 리뷰했습니다. 서로 작업 내용을 카톡으로 공유하고, 막히는 부분은 서로 이야기하며 해결했고, 술술 작업했습니다.
정성훈
-[리팩토링]
코드 리뷰 때 언급된 내용을 리팩토링 했습니다.
동적 라우팅을 이용해 상세 페이지를 구현했습니다. URL의 id를 파라미터로 받아와 api를 호출해 정보를 받아왔습니다.
zustand를 이용해 실시간 검색을 구현했습니다. 디바운싱을 적용해 일정한 텀을 두고 api를 호출하도록 했습니다.
useRef를 이용해 구현했습니다.
장자윤
loading.tsx는 라우트 단위의 데이터 로딩만 관리하기 때문에 페이지 내 이미지가 아직 로딩 중일 때도 따로 컴포넌트 처리
입력값 입력 후 삭제했을 때 처음 렌더링과 동일하게 popular movies 렌더링