-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FE] feat: 좌측 상단에 경로를 표시하는 Breadcrumb 컴포넌트 구현 #372
Conversation
…' into fe/feat/360-path-indicator-router
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.
소소하게 코멘트 남겼어요~~
새 컴포넌트 만드느라 고생했어용
const useBreadcrumbPaths = () => { | ||
const location = useLocation(); | ||
|
||
const breadcrumbPaths = [ |
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.
배열은 -List로 만들어주세용
{ pageName: '연결 페이지', path: '/' }, // TODO: 연결 페이지 경로 결정되면 수정 필요 | ||
]; | ||
|
||
if (location.pathname === '/user/review-list') { |
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.
이제 슬슬 경로를 상수로 뺄지 논의해봐야겠네요
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.
상수 처리를.. 안 했군요!🥹
각 페이지별 경로를 상수 처리했습니다:)
// src/constants/routes.ts
export const ROUTES = {
home: '/',
reviewList: 'user/review-list',
reviewWriting: 'user/review-writing',
reviewWritingComplete: 'user/review-writing-complete',
detailedReview: 'user/detailed-review',
};
breadcrumbPaths.push({ pageName: '작성 페이지', path: location.pathname }); | ||
} | ||
|
||
if (location.pathname.includes('/user/detailed-review/')) { |
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.
저라면 상세 리뷰 페이지만 별도의 includes
를 사용해 검사하기보다는 if문이 중첩되더라도 직관적으로 목록 페이지에서 후속 경로가 있냐 없냐를 검사할 것 같아요.
이건 개인 취향이기도 하니 꼭 반영하지 않아도 됩니다~~
const location = useLocation(); | ||
|
||
const breadcrumbPaths = [ | ||
{ pageName: '연결 페이지', path: '/' }, // TODO: 연결 페이지 경로 결정되면 수정 필요 |
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.
breadcrumbPaths에 연결페이지가 기본으로 들어가있네요.
지금은 변경할 필요 없지만, 나중에 페이지가 더 생기는 경우에 breadcrumPaths 빈 배열로 둘지 한번 생각해봐야겠어요.
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.
Breadcrumb 이라는 좋은 명칭을 사용했네!!!
쑤쑤 고생했어~
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.
쑤쑤~ 고생했어요
컨벤션 관련해서 수정할 부분이 있어서 Approve는 잠시 후에 하겠습니다!🔥🔥
const handleNavigation = (path: PathType) => { | ||
if (typeof path === 'number') { | ||
navigate(path); | ||
} else { | ||
navigate(path); | ||
} | ||
}; |
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.
number, string 유연한 대처 좋아요
frontend/src/constants/routes.ts
Outdated
export const ROUTES = { | ||
home: '/', | ||
review_list: 'user/review-list', | ||
review_writing: 'user/review-writing', | ||
review_writing_complete: 'user/review-writing-complete', | ||
detailed_review: 'user/detailed-review', | ||
}; |
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.
컨벤션에 맞게 camelCase
로 수정해주세요!
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.
예리하십니다!!!
export const ROUTES = {
home: '/',
reviewList: 'user/review-list',
reviewWriting: 'user/review-writing',
reviewWritingComplete: 'user/review-writing-complete',
detailedReview: 'user/detailed-review',
};
@@ -10,7 +10,7 @@ const globalStyles = css` | |||
} | |||
|
|||
body { | |||
font-family: 'Pretendard-Regular', 'NanumGothic', 'Noto Sans', sans-serif; | |||
font-family: 'Pretendard-Regular', 'Noto Sans', sans-serif; |
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.
이 부분도 폰트 관련 pr 머지 이후 regular를 지워야 할 것 같아요.
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.
수정 사항 확인했어요!👍
🚀 어떤 기능을 구현했나요 ?
Breadcrumb
컴포넌트를 구현했습니다.useBreadcrumbPaths
훅으로 분리했습니다.🔥 어떻게 해결했나요 ?
paths
prop을 통해Breadcrumb
컴포넌트에 경로 정보를 전달하고, 각 경로를 클릭하면 페이지가 해당 경로로 이동할 수 있도록useNavigate
훅을 사용해 라우터를 연결했습니다.path
타입을string | number
로 설정했지만navigate
호출 시 오버로드 에러가 발생했습니다. 그래서path
가number
일 경우와string
일 경우를 명확히 구분하여 처리했습니다.각 페이지별 경로를 상수 처리했습니다.
경로를 반환하는 로직을
useBreadcrumbPaths
훅으로 분리했습니다. 연결 페이지에서 시작해서 현재 위치에 맞는 경로를 추가합니다.src/index.tsx
에도routes.ts
에서 정의한 상수를 적용했습니다.📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말