feat(header): refine site header actions and mobile navigation#72
Conversation
- unify header actions for desktop and mobile - improve logout button styling for consistency with login - adjust mobile menu behavior and accessibility
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe changes refactor the application layout by extracting header and main container rendering logic from the root layout into dedicated switcher components. A new SiteHeader component is introduced with mobile and desktop variants, while shop-specific header logic is moved to ShopShell. The LogoutButton component is enhanced with an icon-only variant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/app/[locale]/shop/layout.tsx (1)
11-15: Consider extracting the main container classes to avoid duplication.The main container classes
"mx-auto px-6 min-h-[80vh]"are duplicated between this file andfrontend/components/header/HeaderSwitcher.tsx(line 40). Consider extracting these to a shared constant or utility to maintain consistency.🔎 Possible approach
Create a shared constant:
// frontend/lib/constants.ts or similar export const MAIN_CONTAINER_CLASSES = "mx-auto px-6 min-h-[80vh]";Then use it in both locations:
- <main className="mx-auto px-6 min-h-[80vh]">{children}</main> + <main className={MAIN_CONTAINER_CLASSES}>{children}</main>frontend/app/[locale]/layout.tsx (1)
28-28: Consider improving the type assertion.The
as anycast can be replaced with a more type-safe approach using a type guard.🔎 Possible improvement
- if (!locales.includes(locale as any)) notFound(); + if (!locales.includes(locale as (typeof locales)[number])) notFound();Or create a type guard:
function isValidLocale(locale: string): locale is typeof locales[number] { return locales.includes(locale as any); } if (!isValidLocale(locale)) notFound();frontend/components/header/HeaderSwitcher.tsx (1)
8-23: Consider adding edge case handling to isShopPath.The current logic handles the expected cases well (
/shop,/en/shop, etc.), but could potentially match unintended paths like/something/shop. While this seems acceptable given the current routing structure, consider documenting this behavior or adding a more explicit check if needed.💡 Alternative approach (if stricter matching is needed)
function isShopPath(pathname: string): boolean { const segments = pathname.split('/').filter(Boolean); // Match /shop or /[locale]/shop where locale is a 2-letter code return segments[0] === 'shop' || (segments.length >= 2 && segments[0].length === 2 && segments[1] === 'shop'); }This would be more restrictive but may not be necessary for the current use case.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/[locale]/layout.tsx(3 hunks)frontend/app/[locale]/shop/layout.tsx(1 hunks)frontend/components/auth/logoutButton.tsx(1 hunks)frontend/components/header/HeaderSwitcher.tsx(1 hunks)frontend/components/header/SiteHeader.tsx(1 hunks)frontend/components/header/SiteMobileHeader.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/components/header/SiteMobileHeader.tsx (3)
frontend/components/shop/header/mobile-nav.tsx (2)
MobileNav(12-54)MobileNavProps(8-10)frontend/components/shared/LanguageSwitcher.tsx (1)
LanguageSwitcher(8-72)frontend/components/shop/header/theme-toggle.tsx (1)
ThemeToggleButton(8-23)
frontend/components/header/SiteHeader.tsx (4)
frontend/components/shop/header/theme-toggle.tsx (1)
ThemeToggleButton(8-23)frontend/components/shared/LanguageSwitcher.tsx (1)
LanguageSwitcher(8-72)frontend/components/auth/logoutButton.tsx (1)
LogoutButton(12-44)frontend/components/header/SiteMobileHeader.tsx (1)
SiteMobileHeader(17-75)
frontend/app/[locale]/shop/layout.tsx (3)
frontend/components/shop/shop-shell.tsx (1)
ShopShell(11-28)frontend/app/[locale]/shop/admin/layout.tsx (1)
ShopAdminLayout(12-65)frontend/components/shop/shop-header.tsx (1)
Header(14-61)
frontend/components/auth/logoutButton.tsx (1)
frontend/lib/logout.ts (1)
logout(3-9)
🔇 Additional comments (2)
frontend/components/auth/logoutButton.tsx (1)
8-43: LGTM! Clean implementation of icon-only variant.The new
iconOnlyprop provides a flexible rendering option with proper accessibility attributes. The styling is consistent with other header buttons, and both variants handle the logout flow correctly.frontend/components/header/SiteHeader.tsx (1)
24-84: LGTM! Well-structured responsive header.The component cleanly separates desktop and mobile variants with appropriate accessibility attributes. The conditional authentication controls (login vs logout) are implemented correctly for both viewports.
| const SITE_LINKS = [ | ||
| { href: '/q&a', label: 'Q&A' }, | ||
| { href: '/quiz/react-fundamentals', label: 'Quiz' }, | ||
| { href: '/leaderboard', label: 'Leaderboard' }, | ||
| { href: '/blog', label: 'Blog' }, | ||
| { href: '/about', label: 'About' }, | ||
| { href: '/contacts', label: 'Contacts' }, | ||
| { href: '/shop', label: 'Shop' }, | ||
| ] as const; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract SITE_LINKS to a shared constant.
The SITE_LINKS array is duplicated between this file and frontend/components/header/SiteHeader.tsx (lines 14-22). Extract it to a shared constant file to maintain a single source of truth.
🔎 Recommended refactor
Create a shared constants file:
// frontend/lib/navigation.ts
export const SITE_LINKS = [
{ href: '/q&a', label: 'Q&A' },
{ href: '/quiz/react-fundamentals', label: 'Quiz' },
{ href: '/leaderboard', label: 'Leaderboard' },
{ href: '/blog', label: 'Blog' },
{ href: '/about', label: 'About' },
{ href: '/contacts', label: 'Contacts' },
{ href: '/shop', label: 'Shop' },
] as const;Then import and use in both files:
+import { SITE_LINKS } from '@/lib/navigation';
-const SITE_LINKS = [
- { href: '/q&a', label: 'Q&A' },
- ...
-] as const;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/components/header/SiteMobileHeader.tsx around lines 7 to 15 the
SITE_LINKS array is duplicated in SiteHeader.tsx
(frontend/components/header/SiteHeader.tsx lines ~14-22); extract SITE_LINKS to
a shared constant file (e.g., frontend/lib/navigation.ts) by exporting the array
as a named export, then replace the local SITE_LINKS definitions in both
SiteMobileHeader.tsx and SiteHeader.tsx with an import from that new file;
ensure the exported type/const preserves the as const annotation and update
imports to use the new shared constant.
| aria-expanded={open ? 'true' : 'false'} | ||
| aria-controls="site-mobile-nav" | ||
| > | ||
| {open ? <X className="h-5 w-5" /> : <Menu className="h-6 w-6" />} |
There was a problem hiding this comment.
Icon size inconsistency.
The Menu icon uses h-6 w-6 while the X icon uses h-5 w-5. For visual consistency, both icons should use the same dimensions. The shop mobile nav (from frontend/components/shop/header/mobile-nav.tsx) uses h-5 w-5 for both.
🔎 Suggested fix
- {open ? <X className="h-5 w-5" /> : <Menu className="h-6 w-6" />}
+ {open ? <X className="h-5 w-5" /> : <Menu className="h-5 w-5" />}📝 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.
| {open ? <X className="h-5 w-5" /> : <Menu className="h-6 w-6" />} | |
| {open ? <X className="h-5 w-5" /> : <Menu className="h-5 w-5" />} |
🤖 Prompt for AI Agents
In frontend/components/header/SiteMobileHeader.tsx around line 43, the Menu and
X icons use inconsistent sizes (Menu h-6 w-6 vs X h-5 w-5); update the Menu icon
to use h-5 w-5 (to match the X icon and the shop mobile nav convention) so both
icons have identical dimensions and visual consistency.
2 participants
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.