Conversation
There was a problem hiding this comment.
수고하셨씁니다~~~👏🏻👏🏻 코멘트 남겼으니 확인하고 답변 남겨주셍요!!
그리고 현재 pr 에서 변경된 라인 수, 파일 수가 많은 편이라고 생각됩니당

저는 개인적으로 PR의 변경 라인 수는 500~600줄 이하가 적당하다고 생각하는 편입니다!
변경 사항이 많아지면 리뷰하는 데 시간이 오래 걸리고, 리뷰의 질도 떨어질 수 있다고 생각됩니다,,!!
그리고 그 과정에서 충돌도 많이 일으킬 수 있고 그러다보면,, 머지하는 데 오랜 시간이 걸릴 수 있습니당
(물론 저도 작업하다 보면 마음은 급하고,,, 귀찮아서 한 번에 올릴 때도 있긴 하지만요...ㅎㅎ)
그래서 가능하다면 기능별로 작업 범위를 먼저 나눠서 커밋/PR을 분리하시는 걸 추천드려요!
만약 작업하시다가 변경된 내용이 너무 많아졌다면, 일부는 다음 이슈나 PR에서 이어서 작업하셔도 괜찮을 것 같아요 😊
구현하시느라 정말 고생 많으셨어요 !! 💪✨ 남은 작업까지 파이팅임니다
스크롤이 있는 페이지로 넘어갈 때 스크롤이(페이지가) 항상 맨 위에서 시작하는 게 아니라 중간에서 시작되는 경우가 종종 있습니다. 어떻게 고칠 수 있을까요...
이 부분은,,, 머지되면 저도 한 번 확인해볼게요!! 지금 당장은 해결책이 안떠오르네요 ㅠㅠ
src/__mocks/bestCinemaData.ts
Outdated
| export interface BestCinema { | ||
| id: string; | ||
| rank: number; | ||
| imageUrl: string; | ||
| cinemaName: string; | ||
| rating: number; | ||
| reviewCount: number; | ||
| } |
There was a problem hiding this comment.
추후 API 연동 시 변수명은 변경될 수 있지만 이 타입을 그대로 사용할거니까 여기보다는
types 폴더로 분리해두면 더 좋을 것 같아요~!
src/__mocks/cinemaDetail.ts
Outdated
| export type Review = { | ||
| id: number; | ||
| user: string; | ||
| rating: number; | ||
| content: string; | ||
| likes: number; | ||
| tags: string[]; | ||
| movieTitle: string; | ||
| seatInfo: string[]; | ||
| cinemaName: string; // ex) 남양주현대아울렛 스페이스원 (1관) | ||
| imageUrls?: string[]; | ||
| }; |
There was a problem hiding this comment.
이부분도 마찬가지입니당!!
"data": {
"movieSeatInfo": {
"movieTitle": "string",
"theaterName": "string",
"auditoriumName": "string",
"seatNumber": "string"
},
"hashtags": [
{
"id": 0,
"reviewId": 0,
"hashTagId": 0,
"hashTagName": "string"
}
],
"content": "string",
"rating": 0,
"user": {
"userId": 0,
"nickname": "string",
"profileImageUrl": "string"
},
"heartCount": 0,
"createdAt": "2025-07-27T07:14:36.744Z"
},
참고로 말씀드리면 이게 리뷰 상세 조회 api 응답 구조인데용
타입 구조나 필드명을 백엔드와 미리 맞춰두면 나중에 수정할 일이 줄어들어서 관리하기 더 편한 것 같아요!!
(스웨거 보시면 됩니당..)
아니면 나중에.. 자잘하게 수정할게 많아져서 전 좀 귀찮더라구영...
그래서 저는 보통 작업할 때 처음부터 이 구조에 맞게 타입을 만들고, types 폴더에 분리해 둔 뒤,
그 타입을 그대로 사용해서 구현해두면 나중엔 그냥 API 요청 코드만 추가하면 되는 구조라 훨씬 수월하게 작업이 가능했던 것 같습니당!! ( 그래서 api 연결이 필요한 부분은 타입을 여러 군데 정의해두지 않고 분리해둔 타입만 사용하는게 좋아용 )
그리고 전체 구조를 한 번에 정의하기보단,
user, movieSeatInfo처럼 다른 곳에서도 재사용될 수 있는 필드들은 별도 타입으로 분리해서 관리하는 편입니당 :)
예를 들어, 아래처럼요!
// types/user.ts
export interface User {
userId: number;
nickname: string;
profileImageUrl: string;
}
// types/movie.ts
export interface MovieSeatInfo {
movieTitle: string;
theaterName: string;
auditoriumName: string;
seatNumber: string;
}
// types/review.ts
export interface Review {
id: number;
user: User;
movieSeatInfo: MovieSeatInfo;
...
}
더 좋은 구조 방식이 있을 수 있으니 참고만 해주시면 좋을 것 같아요~!!
src/components/review/RatingCard.tsx
Outdated
| const renderStars = (rating: number) => { | ||
| const full = Math.floor(rating); | ||
| const half = rating % 1 >= 0.5; | ||
| const empty = 5 - full - (half ? 1 : 0); | ||
|
|
||
| return ( | ||
| <div className="flex gap-1"> | ||
| {Array(full) | ||
| .fill(0) | ||
| .map((_, idx) => ( | ||
| <StarFill key={`full-${idx}`} className="h-6 w-6 fill-white text-white" /> | ||
| ))} | ||
| {half && <StarHalf className="h-6 w-6 fill-white text-white" />} | ||
| {Array(empty) | ||
| .fill(0) | ||
| .map((_, idx) => ( | ||
| <StarLine key={`empty-${idx}`} className="h-6 w-6 stroke-white text-white" /> | ||
| ))} | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
지금 RatingCard 안에 UI를 담당하는 로직이 두 개(RatingCard, renderStars) 같이 들어가 있어서
별점 렌더링 부분(renderStars)은 별도의 StarRating 컴포넌트로 분리하면 더 명확하고 재사용하기도 편할 것 같아요! 🙌
import { StarFill, StarHalf, StarLine } from '@/assets';
interface StarRatingProps {
rating: number;
}
const StarRating = ({ rating }: StarRatingProps) => {
const full = Math.floor(rating);
const half = rating % 1 >= 0.5;
const empty = 5 - full - (half ? 1 : 0);
return (
<div className="flex gap-1">
{Array(full)
.fill(0)
.map((_, idx) => (
<StarFill key={`full-${idx}`} className="h-6 w-6 fill-white text-white" />
))}
{half && <StarHalf className="h-6 w-6 fill-white text-white" />}
{Array(empty)
.fill(0)
.map((_, idx) => (
<StarLine key={`empty-${idx}`} className="h-6 w-6 stroke-white text-white" />
))}
</div>
);
};
export default StarRating;
요런식으로 분리하고 RatingCard에서는 이렇게 사용하면 좋을 것 같아요!
<StarRating rating={rating} />
src/pages/home/HomePage.tsx
Outdated
| const imgUrl = getRandomImage(375, 210); | ||
|
|
||
| //좋아요 순으로 정렬 | ||
| const popularReviews = [...cinemaReviewsMock].sort((a, b) => b.likes - a.likes).slice(0, 3); |
| <div className="mt-5"> | ||
| <div className="mb-2 flex items-center justify-between"> | ||
| <p className="text-title-3">영화관 리스트</p> | ||
| </div> | ||
| <div className="flex justify-center gap-3"> | ||
| <button | ||
| onClick={() => navigate('/theaters?tab=imax')} | ||
| className="rounded-m flex h-[163px] w-[166px] flex-col items-center bg-gray-800" | ||
| > | ||
| {/*로고 자리*/} | ||
| <div className="mt-5 aspect-square w-[60%] bg-gray-700" /> | ||
| <p className="mt-2 text-xl text-white">IMAX</p> | ||
| </button> | ||
| <button | ||
| onClick={() => navigate('/theaters?tab=dolby')} | ||
| className="rounded-m flex h-[163px] w-[166px] flex-col items-center bg-gray-800" | ||
| > | ||
| {/*로고 자리*/} | ||
| <div className="mt-5 aspect-square w-[60%] bg-gray-700" /> | ||
| <p className="mt-2 text-xl text-white">Dolby Cinema</p> | ||
| </button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
이것도 !! 구조가 반복되니 CinemaTypeButton과 같은 컴포넌트로 분리하는게 좋아보입니당!!
src/pages/home/HomePage.tsx
Outdated
| <button onClick={handleGoToPopular}> | ||
| <ArrowRight className="h-5 w-5 text-gray-500" /> | ||
| </button> |
There was a problem hiding this comment.
버튼 태그로 감싸면 “클릭 가능한 버튼”이라는 의미가 명확하게 드러나는 장점도 있지만,
혹시 그걸 의도하지 않으셨다면 아래처럼 onClick과 cursor-pointer만으로도 클릭 이벤트를 줄 수 있을 것 같아요!!
<ArrowRight className="h-5 w-5 text-gray-500 cursor-pointer" onClick={handleGoToPopular} />
그리고 확인해보니 화살표 색상이 하얀색이던데 text-gray-500를 사용하신 이유가 있나용??
There was a problem hiding this comment.
헉!! 디자인이 변경되었거나 제가 실수한 것 같습니다! 세세히 봐주셔서 감사합니다 수정하겠습니다 <3
src/pages/home/HomePage.tsx
Outdated
| title={cinema.cinemaName} | ||
| rating={cinema.rating} | ||
| reviewCount={cinema.reviewCount} | ||
| onClick={() => navigate(`/theaters/${encodeURIComponent(cinema.cinemaName)}`)} |
There was a problem hiding this comment.
현재는 상영관 이름 기반으로 페이지 이동이 되도록 구현해주신 것 같은데
한글/특수문자 인코딩 이슈나 유지보수 측면에서 다소 불안정할 수 있어서,
보통은 ID 기반 (/theaters/:id) 라우팅 방식을 더 많이 사용합니다!
추후 API 연동 시에도 각 상영관의 auditoriumId를 활용하게 될 텐데,
스웨거 확인해보니 영화관 목록 전체 조회 API 응답이 아래처럼 내려오는 것 같더라구요
"content": [
{
"auditoriumId": "string",
"theaterName": "string"
}
]
따라서 클릭 시에는 예를 들어 /theaters/13 형식으로 이동하고,
상세 페이지에서는 해당 auditoriumId를 사용해 상세 조회 API를 호출하는 방식이 일반적입니다 🙌
리뷰 페이지에서는 잘 구현해주신것 같은데 이 방식처럼 적용하시면 될 것 같아요 👍🏻👍🏻
src/pages/home/ReviewDetail.tsx
Outdated
| {/*유저 정보, 추후 API 연결 시 프로필 사진 받아와서 조건부로...*/} | ||
| <div className="mt-3 flex items-center gap-2"> | ||
| <div className="flex h-8 w-8 items-center justify-center overflow-hidden rounded-full border border-gray-500 bg-gray-950"> | ||
| <DefaultProfile className="h-5 w-5" /> |
There was a problem hiding this comment.
기본 이미지를 컴포넌트로 분리해서,
API 조회 시 프로필 이미지가 없는 경우 또는 에러가 나는 경우에 대비해
기본 이미지가 노출되도록 처리해보는 건 어떨까요??
이렇게 분리하면 마이페이지 등 프로필 사진을 불러와야 하는 곳에서도 재사용하기 좋을 것 같아용!!
import { useState } from 'react';
import Image from '@/components/Image';
import DefaultProfile from '@/assets';
import { cn } from '@/utils/cn';
interface ProfileImageWithFallbackProps {
src?: string | null;
alt?: string;
size?: number;
className?: string;
}
const ProfileImageWithFallback = ({
src,
alt = '프로필 이미지',
size = 40,
className,
}: ProfileImageWithFallbackProps) => {
const [error, setError] = useState(false);
const resolvedSrc = !src || error ? DefaultProfile : src;
return (
<Image
src={resolvedSrc}
alt={alt}
onError={() => setError(true)}
className={cn(`w-[${size}px] h-[${size}px] rounded-full object-cover`, className)}
/>
);
};
export default ProfileImageWithFallback;
이렇게 img 태그 대신 제가 구현해두었던 Image 컴포넌트를 활용하면
IntersectionObserver, 로딩 블러, 에러 처리 등도 그대로 유지되면서 더 안정적으로 이미지를 다룰 수 있을 것 같아요 🙌
사용 시에는 아래처럼 간단하게 사용할 수 있습니다!
<ProfileImageWithFallback
src={user.profileImageUrl}
size={40}
className="border border-gray-500 ... 추가할 스타일이 있다면 추가"
/>
이미지 URL이 있는 경우엔 해당 이미지를,
없는 경우엔 자동으로 기본 이미지로 대체되니 따로 조건 분기 없이 바로 사용할 수 있습니다!
컴포넌트 내부 props 구조나 스타일 방식은 좀 더 고민해보시고,,,, 지민님 스타일에 맞게 자유롭게 바꿔주셔도 됩니당 !!
There was a problem hiding this comment.
오 좋은 거 같습니다! 🫶🫶 이렇게 컴포넌트로 분리해두면 에러 fallback까지 한 번에 처리할 수 있어서 코드도 훨씬 깔끔해질 것 같네요.... 말씀해주신 방식 참고해서 수정해보겠습니다!
| {/*상세 정보*/} | ||
| <div className="flex flex-col gap-y-3 pt-5"> | ||
| <div className="flex items-center gap-4"> | ||
| <Badge type="info" className="h-7 w-[85px] justify-center"> | ||
| 스크린 | ||
| </Badge> | ||
| <span className="text-caption-2 text-white">{cinemaInfo.screenSize}</span> | ||
| </div> | ||
| <div className="flex items-center gap-4"> | ||
| <Badge type="info" className="h-7 w-[85px] justify-center"> | ||
| 영사 포맷 | ||
| </Badge> | ||
| <span className="text-caption-2 text-white">{cinemaInfo.format}</span> | ||
| </div> | ||
| <div className="flex items-center gap-4"> | ||
| <Badge type="info" className="h-7 w-[85px] justify-center"> | ||
| 음향 | ||
| </Badge> | ||
| <span className="text-caption-2 text-white">{cinemaInfo.sound}</span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
이 부분도 반복되는 구조라서 컴포넌트로 분리하거나,
label / value를 배열로 관리해서 map() 함수로 렌더링하는 방식도 괜찮을 것 같아요!
예를 들어 아래처럼 배열로 정리해서 렌더링하면 좋을 것 같아용
const infoList = [
{ label: '스크린', value: cinemaInfo.screenSize },
{ label: '영사 포맷', value: cinemaInfo.format },
{ label: '음향', value: cinemaInfo.sound },
];
| <Button | ||
| onClick={() => { | ||
| setSelectedCinema(name); | ||
| if (!Array.isArray(halls) || halls.length === 0) { | ||
| // (받아온 데이터에서)관이 없는 경우 | ||
| navigate(`/theaters/${encodeURIComponent(name)}`); | ||
| } else if (halls.length === 1) { | ||
| // 관이 1개인 경우 | ||
| navigate(`/theaters/${encodeURIComponent(name)}`); | ||
| } else { | ||
| // 관이 2개 이상인 경우 | ||
| setSelectedHall(null); | ||
| } | ||
| }} | ||
| variant="secondary-assistive" | ||
| color="gray" | ||
| size="lg" | ||
| fontType="title-3" | ||
| className="w-full justify-start rounded-lg text-left" | ||
| selected={isSelected} | ||
| > |
There was a problem hiding this comment.
onClick 내부에서 navigate 관련 분기 처리가 반복되고 있는데
이 로직을 함수로 추출하면 버튼 내부의 코드가 간결해질 것 같습니당 !
함수로 분리한 뒤 버튼 코드는
<Button
onClick={() => handleCinemaClick(name, halls)}
...
>
{name}
</Button>
이런 식으로 작성하는게 가독성이 좋을 것 같아용😄
soyun-git121
left a comment
There was a problem hiding this comment.
그 중간페이지부터 시작하는 문제 단일페이애플리케이션환경에서 나타날수있는문제라고 하는데
import { useEffect } from 'react';
import { useLocation } from 'react-router-dom';
export default function ScrollToTop() {
const { pathname } = useLocation();
useEffect(() => {
window.scrollTo(0, 0);
}, [pathname]);
return null;
}
scrolltotop컴포넌트 위에처럼 만들어서
broweserrouter컴포넌트에 추가하면된다는데
import { BrowserRouter } from 'react-router-dom';
import ScrollToTop from './ScrollToTop';
ReactDOM.render(
<BrowserRouter>
<ScrollToTop />
<App />
</BrowserRouter>,
document.getElementById('root')
);
이런식으로.. 한번 찾아보고 적용해보실 수 있을거같아요..!(안될수도..)
오 그렇군요!! 감사합니다 참고해서 한 번 해보겠습니다!! |
eileen4505
left a comment
There was a problem hiding this comment.
전체적으로 구조와 기능, 스타일링 모두 디테일하게 신경 써주신 점이 느껴졌어요!
컴포넌트 분리와 타입 분리 등은 매우 잘 진행하셨고, 추후 API 연동을 고려한 설계도 훌륭했습니다 👏
특히 Mock 데이터 기반으로 UI부터 슬라이드 처리까지 깔끔하게 구성해주셔서 흐름 파악이 잘 됐어요.
API 구조를 염두에 두고 타입을 정의하신 점도 👍
앞으로는 PR 분리 기준만 조금 더 신경 써주시면,
리뷰 효율성과 코드 퀄리티 모두 훨씬 좋아질 것 같아요 💪
고생 많으셨습니다! 👏🙌

🔍 관련된 이슈
📝 작업 내용
📸 스크린샷
2025-07-26.141627.mp4
🚨 이슈
📣 리뷰 요구사항
✅ 체크리스트