✨Feat: bottom tab bar 구현 및 페이지별 라우팅 간단 적용#39
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/shared/components/tab/BottomNav.tsx (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🏷️ Labeler has automatically applied labels based on your PR title, branch name, or commit message. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/pages/chatbot/index.tsx (1)
1-7: 페이지 컴포넌트 간 코드 중복이 있습니다.ChatBotPage, EventsPage, HomePage, MyPage, RoutePage가 모두 동일한 구조를 가지고 있습니다. 공통 레이아웃 컴포넌트로 추출하면 유지보수성이 향상됩니다.
예시: 공통 레이아웃 컴포넌트
// src/shared/components/layout/PlaceholderPage.tsx export default function PlaceholderPage({ title }: { title: string }) { return ( <div className="flex items-center justify-center min-h-screen text-mint-700 text-2xl font-semibold"> {title} </div> ); }각 페이지에서 사용:
import PlaceholderPage from '@/shared/components/layout/PlaceholderPage'; export default function ChatBotPage() { return <PlaceholderPage title="ChatBot Page" />; }src/shared/components/tab/BottomNav.tsx (2)
36-48: 접근성 개선을 고려해보세요.탭 네비게이션의 접근성을 높이기 위해
aria-label및aria-current속성을 추가하는 것을 권장합니다.<nav className="..." + aria-label="메인 네비게이션" > {NAV_ITEMS.map((item) => { const isActive = router.pathname === item.href; return ( <Link key={item.id} href={item.href} className="flex-1 flex justify-center" + aria-current={isActive ? 'page' : undefined} + aria-label={item.label} > <TabItem label={item.label} icon={item.icon} isActive={isActive} /> </Link> ); })} </nav>
36-37: 중첩 라우트 지원을 고려해보세요.현재 활성 상태 감지는 정확한 경로 일치(
router.pathname === item.href)만 처리합니다. 향후 중첩 라우트가 추가되면 상위 탭이 활성화되지 않을 수 있습니다.예:
/mypage/settings페이지에서 MyPage 탭이 활성화되지 않음필요시 다음과 같이 수정할 수 있습니다:
- const isActive = router.pathname === item.href; + const isActive = item.href === '/' + ? router.pathname === item.href + : router.pathname.startsWith(item.href);현재 PR에서는 중첩 라우트가 없으므로 즉시 변경할 필요는 없습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/pages/_app.tsx(1 hunks)src/pages/chatbot/index.tsx(1 hunks)src/pages/events/index.tsx(1 hunks)src/pages/home/index.tsx(1 hunks)src/pages/mypage/index.tsx(1 hunks)src/pages/route/index.tsx(1 hunks)src/shared/components/tab/BottomNav.tsx(1 hunks)src/shared/components/tab/TabItem.tsx(1 hunks)src/shared/lib/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/shared/components/tab/TabItem.tsx (2)
src/shared/icons/iconNames.ts (1)
IconName(19-19)src/shared/lib/utils.ts (1)
cn(71-73)
src/shared/components/tab/BottomNav.tsx (2)
src/shared/icons/iconNames.ts (1)
IconName(19-19)src/shared/components/tab/TabItem.tsx (1)
TabItem(28-58)
src/pages/_app.tsx (1)
src/shared/components/tab/BottomNav.tsx (1)
BottomNav(21-51)
🔇 Additional comments (5)
src/pages/events/index.tsx (1)
1-7: 코드 구조 확인 완료.이 페이지는 ChatBotPage와 동일한 패턴을 따르고 있습니다. 코드 중복에 대한 리팩토링 제안은 ChatBotPage 리뷰 코멘트를 참고하세요.
src/pages/mypage/index.tsx (1)
1-7: 코드 구조 확인 완료.ChatBotPage와 동일한 패턴입니다. 중복 코드 리팩토링 제안은 ChatBotPage 리뷰를 참고하세요.
src/pages/home/index.tsx (1)
1-7: 코드 구조 확인 완료.동일한 패턴입니다. 중복 코드 관련 제안은 ChatBotPage 리뷰를 참고하세요.
src/pages/route/index.tsx (1)
1-7: 코드 구조 확인 완료.동일한 패턴입니다. 중복 코드 관련 제안은 ChatBotPage 리뷰를 참고하세요.
src/pages/_app.tsx (1)
7-12: 현재 코드베이스에서는 BottomNav 조건부 렌더링이 필요하지 않습니다.검증 결과, 다음을 확인했습니다:
- 현재 페이지 구조: 모든 페이지는 콘텐츠 페이지(home, chatbot, events, mypage, route)이며, 이들은 BottomNav를 표시해야 합니다.
- 로그인/인증 페이지: 존재하지 않습니다.
- 에러 페이지: 존재하지 않습니다.
- auth 관련 파일: 찾을 수 없습니다.
리뷰 코멘트는 현재 존재하지 않는 페이지(로그인, 에러 페이지)를 기준으로 작성되었습니다. 현재 코드베이스 상태에서 모든 기존 페이지는 BottomNav를 표시하는 것이 올바른 구조입니다. 향후 로그인/인증/에러 페이지가 추가될 때 필요에 따라 조건부 렌더링을 고려하면 됩니다.
Likely an incorrect or invalid review comment.
| const NAV_ITEMS: NavItem[] = [ | ||
| { id: 'home', label: 'Home', icon: 'HouseSimple', href: '/' }, | ||
| { id: 'route', label: 'Route', icon: 'MapPin_', href: '/route' }, | ||
| { id: 'events', label: 'Events', icon: 'CalendarBlank', href: '/events' }, | ||
| { id: 'chatbot', label: 'ChatBot', icon: 'ChatCircle', href: '/chatbot' }, | ||
| { id: 'mypage', label: 'MyPage', icon: 'User', href: '/mypage' }, | ||
| ]; |
There was a problem hiding this comment.
Home 라우트 경로 불일치가 있습니다.
Line 14에서 Home의 href가 '/'로 설정되어 있지만, src/pages/home/index.tsx 파일은 Next.js에서 /home 경로로 매핑됩니다. /는 src/pages/index.tsx에 해당합니다.
현재 프로젝트에 src/pages/index.tsx가 없다면 Home 탭을 클릭했을 때 404 에러가 발생하거나 의도하지 않은 페이지로 이동할 수 있습니다.
다음 중 하나를 선택하여 수정하세요:
옵션 1: Home href를 /home으로 변경
- { id: 'home', label: 'Home', icon: 'HouseSimple', href: '/' },
+ { id: 'home', label: 'Home', icon: 'HouseSimple', href: '/home' },옵션 2: src/pages/home/index.tsx를 src/pages/index.tsx로 이동
다음 스크립트로 index.tsx 파일 존재 여부를 확인하세요:
#!/bin/bash
# Description: Check if root index.tsx exists and verify home page routing
# Check for root index page
fd -t f "^index\.tsx$" src/pages -d 1
# Check home directory
fd -t f "index\.tsx" src/pages/home🤖 Prompt for AI Agents
In src/shared/components/tab/BottomNav.tsx around lines 13 to 19, the Home nav
item's href is set to '/' but your Next.js page for Home lives at
src/pages/home/index.tsx (so '/' will 404 or go to a different page); fix by
either changing the Home item's href to '/home' OR moving/renaming
src/pages/home/index.tsx to src/pages/index.tsx so Home maps to '/', then update
imports/routes accordingly and verify by running the provided fd check or your
dev server.
| <nav | ||
| className=" | ||
| fixed bottom-[2rem] left-1/2 -translate-x-1/2 | ||
| w-[calc(100%-48px)] max-w-[42.5rem] | ||
| flex justify-between items-center | ||
| px-[1.3rem] | ||
| py-[1rem] | ||
| rounded-[37.07px] | ||
| bg-mint-50 border border-mint-300 | ||
| " |
There was a problem hiding this comment.
고정 네비게이션에 z-index가 누락되었습니다.
fixed 포지셔닝을 사용하는 네비게이션 바에 z-index가 명시되지 않아 페이지 콘텐츠가 네비게이션 바를 가릴 수 있습니다.
다음과 같이 z-index를 추가하세요:
<nav
className="
fixed bottom-[2rem] left-1/2 -translate-x-1/2
w-[calc(100%-48px)] max-w-[42.5rem]
flex justify-between items-center
px-[1.3rem]
py-[1rem]
rounded-[37.07px]
bg-mint-50 border border-mint-300
+ z-50
"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <nav | |
| className=" | |
| fixed bottom-[2rem] left-1/2 -translate-x-1/2 | |
| w-[calc(100%-48px)] max-w-[42.5rem] | |
| flex justify-between items-center | |
| px-[1.3rem] | |
| py-[1rem] | |
| rounded-[37.07px] | |
| bg-mint-50 border border-mint-300 | |
| " | |
| <nav | |
| className=" | |
| fixed bottom-[2rem] left-1/2 -translate-x-1/2 | |
| w-[calc(100%-48px)] max-w-[42.5rem] | |
| flex justify-between items-center | |
| px-[1.3rem] | |
| py-[1rem] | |
| rounded-[37.07px] | |
| bg-mint-50 border border-mint-300 | |
| z-50 | |
| " | |
| > |
🤖 Prompt for AI Agents
In src/shared/components/tab/BottomNav.tsx around lines 25 to 34, the fixed
bottom navigation lacks a z-index so page content can overlap it; add an
explicit stacking context (for example the Tailwind utility class z-50 or
another project-standard z value) to the nav element’s class list so the nav
always sits above page content while keeping existing layout and styles intact.
| interface TabItemProps extends VariantProps<typeof tabItemStyle> { | ||
| label: string; | ||
| icon: IconName; | ||
| isActive?: boolean; | ||
| onClick?: () => void; | ||
| } |
There was a problem hiding this comment.
props 이름 불일치 문제를 수정하세요.
VariantProps<typeof tabItemStyle>을 확장하면 이미 active prop이 포함되지만, 컴포넌트에서는 isActive를 별도 prop으로 선언했습니다. 이로 인해 두 prop이 모두 존재하게 되어 혼란을 초래할 수 있습니다.
다음 중 하나를 선택하여 수정하세요:
방법 1 (권장): cva 변형 prop을 직접 사용
-interface TabItemProps extends VariantProps<typeof tabItemStyle> {
+interface TabItemProps
+ extends Omit<VariantProps<typeof tabItemStyle>, 'active'> {
label: string;
icon: IconName;
isActive?: boolean;
onClick?: () => void;
}방법 2: cva 변형 이름을 일치시키기
tabItemStyle의 변형을 active에서 isActive로 변경:
const tabItemStyle = cva(
'...',
{
variants: {
- active: {
+ isActive: {
true: 'text-mint-700',
false: 'text-mint-500',
},
},
defaultVariants: {
- active: false,
+ isActive: false,
},
},
);그런 다음 컴포넌트에서:
- className={cn(tabItemStyle({ active: isActive }))}
+ className={cn(tabItemStyle({ isActive }))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface TabItemProps extends VariantProps<typeof tabItemStyle> { | |
| label: string; | |
| icon: IconName; | |
| isActive?: boolean; | |
| onClick?: () => void; | |
| } | |
| interface TabItemProps | |
| extends Omit<VariantProps<typeof tabItemStyle>, 'active'> { | |
| label: string; | |
| icon: IconName; | |
| isActive?: boolean; | |
| onClick?: () => void; | |
| } |
🤖 Prompt for AI Agents
In src/shared/components/tab/TabItem.tsx around lines 21 to 26, the interface
currently extends VariantProps<typeof tabItemStyle> (which already defines an
active prop) but also declares a separate isActive prop, causing
duplicate/conflicting props; fix by removing isActive from the interface and
using the cva variant prop name (active) throughout the component and its
callers (or alternatively rename the cva variant to isActive if you prefer that
naming), ensure the component passes the active boolean into tabItemStyle
invocation and update any parent usages to supply active instead of isActive so
only one prop controls the active state.
| <div | ||
| className={cn(tabItemStyle({ active: isActive }))} | ||
| onClick={onClick} | ||
| > | ||
| <Icon | ||
| name={icon} | ||
| size={24} | ||
| color={isActive ? 'mint-700' : 'mint-500'} | ||
| className={cn( | ||
| 'transition-transform duration-200', | ||
| isActive && 'scale-110', | ||
| )} | ||
| /> | ||
| <span | ||
| className={cn( | ||
| 'text-label-md font-medium', | ||
| isActive ? 'text-mint-700' : 'text-mint-500', | ||
| )} | ||
| > | ||
| {label} | ||
| </span> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
접근성 문제: 버튼 요소를 사용하거나 적절한 키보드 지원을 추가하세요.
클릭 가능한 div 요소를 사용하면 키보드 탐색이 불가능하고 스크린 리더 사용자가 탭 버튼임을 인식할 수 없습니다. 이는 접근성 기준(WCAG)을 위반합니다.
button 요소를 사용하도록 수정하세요:
return (
- <div
- className={cn(tabItemStyle({ active: isActive }))}
+ <button
+ type="button"
+ className={cn(tabItemStyle({ active: isActive }))}
onClick={onClick}
+ aria-label={label}
+ aria-current={isActive ? 'page' : undefined}
>
<Icon
name={icon}
size={24}
color={isActive ? 'mint-700' : 'mint-500'}
className={cn(
'transition-transform duration-200',
isActive && 'scale-110',
)}
/>
<span
className={cn(
'text-label-md font-medium',
isActive ? 'text-mint-700' : 'text-mint-500',
)}
>
{label}
</span>
- </div>
+ </button>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div | |
| className={cn(tabItemStyle({ active: isActive }))} | |
| onClick={onClick} | |
| > | |
| <Icon | |
| name={icon} | |
| size={24} | |
| color={isActive ? 'mint-700' : 'mint-500'} | |
| className={cn( | |
| 'transition-transform duration-200', | |
| isActive && 'scale-110', | |
| )} | |
| /> | |
| <span | |
| className={cn( | |
| 'text-label-md font-medium', | |
| isActive ? 'text-mint-700' : 'text-mint-500', | |
| )} | |
| > | |
| {label} | |
| </span> | |
| </div> | |
| ); | |
| <button | |
| type="button" | |
| className={cn(tabItemStyle({ active: isActive }))} | |
| onClick={onClick} | |
| aria-label={label} | |
| aria-current={isActive ? 'page' : undefined} | |
| > | |
| <Icon | |
| name={icon} | |
| size={24} | |
| color={isActive ? 'mint-700' : 'mint-500'} | |
| className={cn( | |
| 'transition-transform duration-200', | |
| isActive && 'scale-110', | |
| )} | |
| /> | |
| <span | |
| className={cn( | |
| 'text-label-md font-medium', | |
| isActive ? 'text-mint-700' : 'text-mint-500', | |
| )} | |
| > | |
| {label} | |
| </span> | |
| </button> | |
| ); |
| <Icon | ||
| name={icon} | ||
| size={24} | ||
| color={isActive ? 'mint-700' : 'mint-500'} | ||
| className={cn( | ||
| 'transition-transform duration-200', | ||
| isActive && 'scale-110', | ||
| )} | ||
| /> | ||
| <span | ||
| className={cn( | ||
| 'text-label-md font-medium', | ||
| isActive ? 'text-mint-700' : 'text-mint-500', | ||
| )} | ||
| > | ||
| {label} | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
색상 로직 중복을 제거하세요.
색상 결정 로직이 세 곳에서 반복됩니다:
tabItemStyle변형 (line 11-12)Icon의colorprop (line 42)span의className(line 51)
부모 div의 text-mint-700/text-mint-500 클래스가 이미 자식 요소에 상속되므로 중복이 불필요합니다.
다음과 같이 리팩토링하세요:
return (
<div
className={cn(tabItemStyle({ active: isActive }))}
onClick={onClick}
>
<Icon
name={icon}
size={24}
- color={isActive ? 'mint-700' : 'mint-500'}
className={cn(
'transition-transform duration-200',
isActive && 'scale-110',
)}
/>
<span
- className={cn(
- 'text-label-md font-medium',
- isActive ? 'text-mint-700' : 'text-mint-500',
- )}
+ className="text-label-md font-medium"
>
{label}
</span>
</div>
);참고: Icon 컴포넌트가 부모로부터 색상을 상속받지 않는다면, color prop은 유지하되 span의 중복된 색상 클래스는 제거하세요.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Icon | |
| name={icon} | |
| size={24} | |
| color={isActive ? 'mint-700' : 'mint-500'} | |
| className={cn( | |
| 'transition-transform duration-200', | |
| isActive && 'scale-110', | |
| )} | |
| /> | |
| <span | |
| className={cn( | |
| 'text-label-md font-medium', | |
| isActive ? 'text-mint-700' : 'text-mint-500', | |
| )} | |
| > | |
| {label} | |
| </span> | |
| <Icon | |
| name={icon} | |
| size={24} | |
| className={cn( | |
| 'transition-transform duration-200', | |
| isActive && 'scale-110', | |
| )} | |
| /> | |
| <span | |
| className="text-label-md font-medium" | |
| > | |
| {label} | |
| </span> |
| text: [ | ||
| 'text-display-lg', | ||
| 'text-display-md', | ||
| 'text-display-sm', | ||
| 'text-headline-lg', | ||
| 'text-headline-md', | ||
| 'text-headline-sm', | ||
| 'text-title-lg', | ||
| 'text-title-md', | ||
| 'text-title-sm', | ||
| 'text-label-lg', | ||
| 'text-label-md', | ||
| 'text-label-sm', | ||
| 'text-body-lg', | ||
| 'text-body-md', | ||
| 'text-body-sm', | ||
| 'text-display-serif', | ||
| 'text-headline-lg-serif', | ||
| 'text-headline-md-serif', | ||
| 'text-headline-sm-serif', | ||
| 'text-label-serif', | ||
| 'display-lg', | ||
| 'display-md', | ||
| 'display-sm', | ||
| 'headline-lg', | ||
| 'headline-md', | ||
| 'headline-sm', | ||
| 'title-lg', | ||
| 'title-md', | ||
| 'title-sm', | ||
| 'label-lg', | ||
| 'label-md', | ||
| 'label-sm', | ||
| 'body-lg', | ||
| 'body-md', | ||
| 'body-sm', | ||
| 'display-serif', | ||
| 'headline-lg-serif', | ||
| 'headline-md-serif', | ||
| 'headline-sm-serif', | ||
| 'label-serif', | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
타이포그래피 키 변경 사항을 전체 코드베이스에서 확인하세요.
text- 접두사가 제거되었습니다 (예: text-display-lg → display-lg). 이는 tailwind-merge 설정 변경이므로, 실제 Tailwind CSS 클래스명이 프로젝트의 tailwind.config.ts에서도 동일하게 업데이트되었는지 확인이 필요합니다.
또한 기존 컴포넌트에서 이전 키를 사용하고 있다면 업데이트가 필요합니다.
다음 스크립트로 검증하세요:
🏁 Script executed:
#!/bin/bash
# Description: Verify typography key usage across the codebase
echo "=== Checking tailwind.config.ts for typography definitions ==="
fd "tailwind.config" --exec cat {}
echo -e "\n=== Searching for old text- prefixed typography classes ==="
rg -n "text-(display|headline|title|label|body)-(lg|md|sm)" src/ \
-g "*.tsx" -g "*.ts" -g "!*.config.ts" \
--context 2
echo -e "\n=== Searching for new typography classes without text- prefix ==="
rg -n "(display|headline|title|label|body)-(lg|md|sm)" src/ \
-g "*.tsx" -g "*.ts" -g "!utils.ts" \
--context 1Length of output: 777
추가 검증이 필요합니다. 타이포그래피 마이그레이션의 완성도를 확인하겠습니다.
추가 검증을 진행하겠습니다. utils.ts 파일의 내용과 전체 코드베이스에서의 사용 패턴을 확인하겠습니다.
타이포그래피 마이그레이션이 미완료 상태입니다. 컴포넌트 업데이트가 필요합니다.
검증 결과, src/shared/components/tab/TabItem.tsx 라인 50에서 여전히 이전 형식(text-label-md)을 사용하고 있습니다. src/shared/lib/utils.ts에서 정의한 새로운 형식(접두사 제거)으로 업데이트되지 않았습니다.
- TabItem.tsx 라인 50:
'text-label-md'→'label-md'로 변경 필요 tailwind.config.ts파일을 찾을 수 없어 실제 Tailwind 클래스 정의 상태 확인 불가- 코드베이스의 다른 컴포넌트에서도 동일한 이전 형식을 사용하고 있을 가능성 검토 필요
🤖 Prompt for AI Agents
In src/shared/lib/utils.ts around lines 7-28 the typography tokens were migrated
to remove the "text-" prefix (e.g., 'label-md'), but a component still uses the
old format: update src/shared/components/tab/TabItem.tsx at line 50 replacing
'text-label-md' with 'label-md'; then sweep the codebase for other occurrences
of "text-" prefixed typography classes (e.g., text-*-*) and replace them with
the new token names from utils.ts, and if needed verify tailwind.config.ts to
ensure the new class names are defined (the config file wasn't found during
review).
jjangminii
left a comment
jjangminii
left a comment
There was a problem hiding this comment.
저희 배럴파일은 지양하기로 했어서 이부분은 나중에 페이지 만들게된다면 수정하는게 좋을 것. ㅏㅌ아요 추가적으로 코멘트 남겼으니 확인해주세요-! 수고하셨습니다
| flex justify-between items-center | ||
| px-[1.3rem] | ||
| py-[1rem] | ||
| rounded-[37.07px] |
There was a problem hiding this comment.
rounded 피그마 보니까 그렇게 되있더라고요..
| <nav | ||
| className=" | ||
| fixed bottom-[2rem] left-1/2 -translate-x-1/2 | ||
| w-[calc(100%-48px)] max-w-[42.5rem] |
There was a problem hiding this comment.
w-full로 주고 사용하는곳에서 패딩값으로 조절하는 방식은 어떤가요? 그리고 최소 사이즈를 지정하는 방식으로요
There was a problem hiding this comment.
그렇게 히면 이게 fixed여서 패딩 없이 꽉 채우더라고요..
| isActive?: boolean; | ||
| onClick?: () => void; |
There was a problem hiding this comment.
이 두개가 사용되지 않을 때가 있나요? 옵셔널로 한 이유가 있을까요
| const NAV_ITEMS: NavItem[] = [ | ||
| { id: 'home', label: 'Home', icon: 'HouseSimple', href: '/' }, | ||
| { id: 'route', label: 'Route', icon: 'MapPin_', href: '/route' }, | ||
| { id: 'events', label: 'Events', icon: 'CalendarBlank', href: '/events' }, | ||
| { id: 'chatbot', label: 'ChatBot', icon: 'ChatCircle', href: '/chatbot' }, | ||
| { id: 'mypage', label: 'MyPage', icon: 'User', href: '/mypage' }, | ||
| ]; |
There was a problem hiding this comment.
이 부분은 따로 상수나 타입파일로 분리하는건 어떤가요?
| @@ -0,0 +1,51 @@ | |||
| import Link from 'next/link'; | |||
| import { useRouter } from 'next/router'; | |||
| import { TabItem } from './TabItem'; | |||
There was a problem hiding this comment.
동일 뎁스에 있는 파일이라 상대경로가 간단해보이긴하지만 절대경로로 수정하는 건 어떤가요? 이 부분에 대해서 어떻게 생각하시는지 궁금합니다
There was a problem hiding this comment.
일단 구현해놓고 절대경로 수정하려고 했습니다! 수정할게요
KongMezu
left a comment
KongMezu
left a comment
There was a problem hiding this comment.
시험기간에도 고생많으셨습니다.. 시험 파이팅하시고 나중에 리뷰 한번 보심 좋을거 같습니다 :)
| " | ||
| > | ||
| {NAV_ITEMS.map((item) => { | ||
| const isActive = router.pathname === item.href; |
There was a problem hiding this comment.
이거 하위 페이지로 진입하면 상위 탭 꺼지지 않을까요?? startwith 써서 코드 수정 해보면 좋을 거 같습니다 :)
|
화면 하단에 여백이 없어서 윗 컴포넌트(내브바)랑 아래 컴포넌트(본 화면)가 겹치더라고요.. 그래서 이런 식으로 했는데 괜찮은가요?? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/shared/components/tab/TabItem.tsx (3)
39-55: 색상 로직 중복 제거 권장.색상 결정 로직이 세 곳에서 반복됩니다:
tabItemStyle변형 (lines 11-12)Icon의colorprop (line 42)span의className(line 51)부모 요소의
text-mint-700/text-mint-500클래스가 자식에게 상속되므로 중복이 불필요합니다.다음과 같이 리팩토링할 수 있습니다:
<Icon name={icon} size={24} - color={isActive ? 'mint-700' : 'mint-500'} className={cn( 'transition-transform duration-200', isActive && 'scale-110', )} /> <span - className={cn( - 'text-body-sm', - isActive ? 'text-mint-700' : 'text-mint-500', - )} + className="text-body-sm" > {label} </span>참고:
Icon컴포넌트가 부모의 색상을 상속받지 않는다면colorprop은 유지하되,span의 중복된 색상 클래스는 제거하세요.
21-26: prop 이름 충돌과 불필요한 옵셔널 수정 필요.이전 리뷰에서 지적된 두 가지 문제가 여전히 남아있습니다:
VariantProps<typeof tabItemStyle>이 이미activeprop을 포함하는데 별도로isActive를 선언하여 충돌이 발생합니다.- @jjangminii의 질문에 답변: 하단 네비게이션 컨텍스트에서
isActive와onClick은 항상 제공되므로 옵셔널로 만들 필요가 없습니다. 옵셔널로 두면 런타임에 문제가 발생할 수 있습니다.다음과 같이 수정하세요:
-interface TabItemProps extends VariantProps<typeof tabItemStyle> { +interface TabItemProps { label: string; icon: IconName; - isActive?: boolean; - onClick?: () => void; + isActive: boolean; + onClick: () => void; }
35-57: 접근성 문제: button 요소를 사용하세요.이전 리뷰에서 지적된 접근성 문제가 해결되지 않았습니다. 클릭 가능한
div대신button요소를 사용해야 합니다:
- 키보드 탐색 불가능 (Tab, Enter, Space 키 미지원)
- 스크린 리더가 탭 버튼임을 인식하지 못함
- WCAG 접근성 기준 위반
다음과 같이 수정하세요:
return ( - <div + <button + type="button" className={cn(tabItemStyle({ active: isActive }))} onClick={onClick} + aria-label={label} + aria-current={isActive ? 'page' : undefined} > <Icon name={icon} size={24} color={isActive ? 'mint-700' : 'mint-500'} className={cn( 'transition-transform duration-200', isActive && 'scale-110', )} /> <span className={cn( 'text-body-sm', isActive ? 'text-mint-700' : 'text-mint-500', )} > {label} </span> - </div> + </button> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/shared/components/tab/BottomNav.tsx(1 hunks)src/shared/components/tab/TabItem.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/components/tab/BottomNav.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/shared/components/tab/TabItem.tsx (2)
src/shared/icons/iconNames.ts (1)
IconName(22-22)src/shared/lib/utils.ts (1)
cn(71-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
src/shared/components/tab/TabItem.tsx (2)
1-4: LGTM!필요한 모든 의존성이 올바르게 임포트되었습니다.
6-19: 스타일 정의는 요구사항을 충족하지만 prop 이름과의 불일치를 확인하세요.cva 변형이
active로 정의되어 있지만, 컴포넌트에서는isActiveprop을 사용합니다. 이로 인해 lines 21-26에서 prop 타입 충돌이 발생합니다.
| /> | ||
| <span | ||
| className={cn( | ||
| 'text-body-sm', |
There was a problem hiding this comment.
🧩 Analysis chain
타이포그래피 클래스 확인 필요.
이전 리뷰 코멘트에서 text-label-md를 제안했는데 현재 text-body-sm을 사용하고 있습니다. 디자인 시스템 명세에 맞는지 확인하세요.
I'll verify the typography class usage by examining the file and design system specifications.
🏁 Script executed:
#!/bin/bash
# Examine the TabItem.tsx file around line 50 with context
cat src/shared/components/tab/TabItem.tsx | head -80 | tail -40Length of output: 790
🏁 Script executed:
#!/bin/bash
# Search for design system or typography class definitions
find . -type f \( -name "*.tsx" -o -name "*.ts" -o -name "*.css" -o -name "*.scss" \) | xargs rg -l "text-body-sm|text-label-md" -i | head -20Length of output: 127
🏁 Script executed:
#!/bin/bash
# Look for design system documentation or tailwind config
find . -type f \( -name "*tailwind*" -o -name "*theme*" -o -name "*design*" -o -name "*token*" \) | grep -E "\.(config|json|ts|js)$" | head -20Length of output: 930
🏁 Script executed:
#!/bin/bash
# Search for the typography classes in the entire codebase to understand usage patterns
rg "text-label-md|text-body-sm" -n --type=tsx --type=ts -C 2 | head -50Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the globals.css file for typography class definitions
cat src/styles/globals.css | grep -A 5 -B 2 "text-body-sm\|text-label-md"
Length of output: 1282
🏁 Script executed:
#!/bin/bash
# Search for tailwind config file
fd "tailwind" --extension "config" --extension "js" --extension "ts" --extension "json"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at Tag.tsx to see how similar components use typography
cat src/shared/components/tag/Tag.tsx | head -100Length of output: 1026
🏁 Script executed:
#!/bin/bash
# Search for typography usage with corrected syntax
rg "text-label-md|text-body-sm" -n -C 3 2>/dev/nullLength of output: 3265
text-label-md로 변경이 필요합니다.
디자인 시스템 명세에 따르면:
text-label-md: font-weight 500 (UI 레이블/요소용)text-body-sm: font-weight 400 (본문 텍스트용)
TabItem의 레이블은 네비게이션 요소이므로, Tag 컴포넌트와 마찬가지로 text-label-md를 사용해야 합니다. 현재 text-body-sm은 디자인 시스템 명세와 맞지 않습니다.
🤖 Prompt for AI Agents
In src/shared/components/tab/TabItem.tsx around line 50, the text size/weight
class currently uses 'text-body-sm' but must be changed to 'text-label-md' to
match the design system (Tab labels are navigation elements and should use
font-weight 500 like Tag); update the className where 'text-body-sm' appears to
'text-label-md' (keeping other classes intact) and save/verify styles compile
and visual spot-check against the Tag component.
KongMezu
left a comment
KongMezu
left a comment
There was a problem hiding this comment.
수고하셨습니다! 스크린리더 향상을 위한 리뷰 하나 남겨놨으니 한 번 적용해보시는게 좋을 거 같습니다 :) approve 해두겠습니다
| router.pathname === item.href || router.pathname.startsWith(item.href + '/'); | ||
|
|
||
| return ( | ||
| <Link |
There was a problem hiding this comment.
저희가 스크린리더도 신경쓰기로 했으니
aria-current 추가해 접근성을 향상하면 좋을거 같습니다!
3 participants
🔥 작업 내용
🤔 추후 작업 사항
🔗 이슈
PR Point (To Reviewer)
참고 사항
📸 피그마 스크린샷 or 기능 GIF
(작업 내역 스크린샷)
2025-10-18.043343.mp4
Summary by CodeRabbit
릴리스 노트