-
Notifications
You must be signed in to change notification settings - Fork 14
navbar herosection #60
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
WalkthroughThe PR restructures the hero section with simplified, responsive typography to address mobile text cropping issues, and reworks the navigation component to include a collapsible mobile menu with responsive layout adjustments and a reusable NavButton component for consistency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
components/navigation.tsx (3)
10-12: Consider removing or simplifyinggetImagePathhelper.The function now simply returns the path unchanged (just normalizing a leading slash). This appears to be vestigial from a previous implementation. If no path transformation is needed, consider removing it entirely and using paths directly, or add a comment explaining when this helper would be needed.
60-68: Improve hamburger button accessibility.The button lacks state indication for screen readers and doesn't support closing the menu via keyboard (Escape key). The visual state also doesn't change when the menu is open.
Apply this diff to improve accessibility:
{/* Hamburger */} <button onClick={() => setOpen(!open)} className="md:hidden flex flex-col justify-center gap-1.5 p-2" - aria-label="Open menu" + aria-label={open ? "Close menu" : "Open menu"} + aria-expanded={open} > - <span className="w-6 h-0.5 bg-gray-800" /> - <span className="w-6 h-0.5 bg-gray-800" /> - <span className="w-6 h-0.5 bg-gray-800" /> + <span className={`w-6 h-0.5 bg-gray-800 transition-transform ${open ? 'rotate-45 translate-y-2' : ''}`} /> + <span className={`w-6 h-0.5 bg-gray-800 transition-opacity ${open ? 'opacity-0' : ''}`} /> + <span className={`w-6 h-0.5 bg-gray-800 transition-transform ${open ? '-rotate-45 -translate-y-2' : ''}`} /> </button>Additionally, consider adding an effect to close the menu on Escape key press:
useEffect(() => { const handleEscape = (e: KeyboardEvent) => { if (e.key === 'Escape') setOpen(false) } if (open) document.addEventListener('keydown', handleEscape) return () => document.removeEventListener('keydown', handleEscape) }, [open])
74-92: Consider closing mobile menu on route change.When a user navigates via
MobileLink, the menu closes due to theonClickhandler. However, if navigation occurs through other means (e.g., browser back/forward), the menu would remain open.Consider adding an effect to close the menu when the pathname changes:
useEffect(() => { setOpen(false) }, [pathname])app/page.tsx (2)
219-284: Fix inconsistent indentation in hero section.The hero section JSX has inconsistent indentation (lines 219-284 appear to have extra leading spaces compared to the surrounding code structure). This affects code readability and maintainability.
The content within
<div className="space-y-6">should be indented consistently with the rest of the file. Consider reformatting this block to match the 2-space indentation used elsewhere:<div className="space-y-6"> - <div className="relative"> - {/* Main heading with enhanced styling */} - <div className="relative z-10 + <div className="relative"> + {/* Main heading with enhanced styling */} + <div className="relative z-10 ...
232-241: Remove emptyclassNameprops from DecryptedText components.The
className=""props are unnecessary and add noise to the code. Based on the DecryptedText component in the relevant snippets, these props are optional.<DecryptedText text="Welcome to the On-Chain" animateOn="view" speed={75} maxIterations={15} sequential={true} revealDirection="start" - className="" encryptedClassName="text-amber-400/40" />Apply similar changes to the other DecryptedText instances at lines 248 and 269.
Also applies to: 248-257, 269-278
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/page.tsx(1 hunks)components/navigation.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/page.tsx (1)
components/DecryptedText.tsx (1)
DecryptedText(35-239)
components/navigation.tsx (1)
components/ui/button.tsx (1)
Button(56-56)
🔇 Additional comments (6)
components/navigation.tsx (3)
41-51: LGTM on desktop navigation structure.The NavButton abstraction provides clean, reusable styling with proper active state highlighting. Good use of responsive hiding with
hidden md:flex.
99-122: NavButton implementation is clean.Good separation of concerns with proper TypeScript typing. The conditional styling for active state is clear and the component is appropriately scoped as a non-exported helper.
124-142: MobileLink implementation is straightforward.Clean implementation with proper click handler for closing the menu. The styling is consistent with the overall design system.
app/page.tsx (3)
212-212: LGTM on container adjustment.Removing padding/margin from the root container (
px-0 mx-0) allows the hero section to utilize full width appropriately.
225-259: Responsive typography addresses the hero text cropping issue.Good implementation of responsive breakpoints (
text-4xl sm:text-5xl md:text-6xl lg:text-7xl) to ensure text scales properly across screen sizes. Themax-w-full break-wordson the h1 element provides a fallback for edge cases.
221-223: Verify negative margin behavior on different screen sizes.The negative top margins (
-mt-16 lg:-mt-24) shift content upward. Ensure this doesn't cause content to overlap with the navigation or get clipped on various screen sizes, especially when combined with the sticky header.
This pull request improves the responsiveness of the navigation bar and hero section, especially on small and mobile screens.
Changes made:
These changes improve overall mobile usability and ensure a consistent experience across different screen sizes.
Fixes #49
Fixes #50
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.