-
Notifications
You must be signed in to change notification settings - Fork 12
fix: improved responsiveness and added a hamburger #49
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
|
Someone is attempting to deploy a commit to the rishabh's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor License Agreement (CLA)Thank you for your contribution to ToolBox. By submitting this pull request, you automatically agree to our Contributor License Agreement (CLA). Key Terms
Full AgreementPlease review the complete CLA for all terms and conditions. QuestionsIf you have questions about the CLA, please comment on this pull request or open a discussion. By continuing with this pull request, you confirm your acceptance of the CLA. |
WalkthroughComprehensive responsive redesign of the homepage and header component, introducing mobile-first styling with adjusted breakpoints, reduced spacing for compact layouts, and a new mobile navigation sheet. Both files apply consistent responsive patterns across sections for improved mobile readability. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
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: 3
🧹 Nitpick comments (1)
components/HomeHeader.tsx (1)
132-190: Consider extracting navigation items into shared utilities.The mobile Sheet (Lines 132-190) and desktop navigation (Lines 43-118) duplicate authentication logic and link structures. This makes maintenance harder and increases the risk of inconsistencies.
Consider extracting navigation items into a shared array or component:
// Example structure const navigationItems = { authenticated: (session) => [ { label: 'Tools', href: '/tools' }, ...(session.user?.isAdmin ? [{ label: 'Dashboard', href: '/admin' }] : []), // etc. ], unauthenticated: [ { label: 'Sign In', href: '/login', icon: LogIn }, { label: 'Get Started', href: '/register', icon: UserPlus }, ] };This would centralize navigation logic and ensure consistency across mobile and desktop views.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/page.tsx(3 hunks)components/HomeHeader.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/page.tsx (2)
components/ui/button.tsx (1)
Button(56-56)lib/tools.ts (1)
tools(11-60)
🔇 Additional comments (9)
components/HomeHeader.tsx (3)
6-6: LGTM!The new imports for the mobile navigation are correctly added.
Also applies to: 16-22
34-40: LGTM!The responsive container padding and brand styling improvements enhance mobile usability.
123-131: Good accessibility practices.The mobile menu includes proper screen reader text (Line 125) and a Sheet title (Line 130) for assistive technologies.
app/page.tsx (6)
70-74: LGTM! Comprehensive responsive typography.The hero typography scales appropriately across breakpoints, creating a strong visual hierarchy. The text-7xl on xl+ screens is bold but aligns with modern landing page design.
97-125: LGTM! Well-structured responsive tool cards.The Featured Tools section includes proper responsive padding, grid gaps, and icon sizing. Line 116's
min-w-0is a good practice for enabling text truncation in flex containers.
128-152: LGTM! Thoughtful mobile-to-desktop layout transition.The Features section properly transitions from centered mobile layout to left-aligned desktop layout (Lines 143, 146-147), providing optimal readability across screen sizes.
155-169: LGTM! Clean CTA section with responsive sizing.The CTA section includes appropriate responsive padding and button sizing. Note that Line 162 also uses the Link-wrapping-Button pattern mentioned earlier.
172-198: LGTM! Well-structured responsive footer.The footer properly stacks on mobile (Line 174) and includes appropriate responsive text sizing and visual separators (Line 183).
61-198: Excellent mobile-first responsive implementation.The landing page demonstrates consistent responsive patterns throughout:
- Uniform breakpoint usage (sm:, md:, lg:, xl:)
- Progressive enhancement from mobile to desktop
- Consistent spacing and typography scales
- Proper mobile-to-desktop layout transitions
This aligns well with the mobile navigation improvements in HomeHeader.tsx.
| <Link href="/tools" className="w-full sm:w-auto"> | ||
| <Button size="lg" className="w-full sm:w-auto h-11 sm:h-12 px-6 group text-sm sm:text-base"> | ||
| Explore Tools | ||
| <ArrowRight className="ml-2 h-5 w-5 transition-transform group-hover:translate-x-1" /> | ||
| <ArrowRight className="ml-2 h-4 w-4 sm:h-5 sm:w-5 transition-transform group-hover:translate-x-1" /> | ||
| </Button> | ||
| </Link> | ||
| <Link href="https://github.com/rishabh3562/ToolBox" target="_blank"> | ||
| <Button size="lg" variant="outline" className="h-12 px-6"> | ||
| <Link href="https://github.com/rishabh3562/ToolBox" target="_blank" className="w-full sm:w-auto"> | ||
| <Button size="lg" variant="outline" className="w-full sm:w-auto h-11 sm:h-12 px-6 text-sm sm:text-base"> | ||
| View on GitHub | ||
| </Button> | ||
| </Link> |
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.
🧩 Analysis chain
🏁 Script executed:
fd -t f -n "button.tsx" components/Repository: rishabh3562/ToolBox
Length of output: 182
🌐 Web query:
shadcn/ui Button component asChild prop Link wrapper
💡 Result:
Short answer — use Button's asChild (which uses Radix Slot) to make a Link render with button styles; e.g.:
-
simple (recommended):
Login
import Link from "next/link"
import { Button } from "@/components/ui/button"
(Shadcn docs example). [1]
Notes / gotchas
- asChild is implemented with @radix-ui/react-slot, so your child component must accept/spread forwarded props and ref (Radix docs). If the child doesn't forward props/refs the composition will break. [2]
- If you need to style a Link without using asChild you can apply buttonVariants/classes directly to the Link. [3]
- There have been recent reported issues where certain versions of @radix-ui/react-slot break Next.js Link + asChild; if you hit that, either apply button classes on the Link (buttonVariants) or wrap an inside Link / use a Link that forwards props correctly while you wait for a fix. See the GitHub issue. [4]
Sources:
- Shadcn Button docs (asChild example). [1]
- Radix Slot / asChild composition guidance. [2]
- shadcn docs showing buttonVariants alternative. [3]
- GitHub issue reporting Next.js Link + Slot problem. [4]
Restructure Button and Link composition to avoid nested interactive elements.
Lines 80 and 86 incorrectly wrap <Button> inside <Link>, creating nested interactive elements. Use the asChild prop (from shadcn/ui's built-in support) to properly compose them:
<Button asChild size="lg" className="w-full sm:w-auto h-11 sm:h-12 px-6 group text-sm sm:text-base">
<Link href="/tools">
Explore Tools
<ArrowRight className="ml-2 h-4 w-4 sm:h-5 sm:w-5 transition-transform group-hover:translate-x-1" />
</Link>
</Button>Apply the same fix to line 86 for the GitHub link.
🤖 Prompt for AI Agents
In app/page.tsx around lines 80-90 the Button components are wrapped by Link
creating nested interactive elements; replace that composition by using Button's
asChild prop and place the Link inside the Button for both the "/tools" and the
external GitHub link, i.e., change to <Button asChild ...><Link
href="/tools">...</Link></Button> and for the GitHub link ensure the inner Link
keeps target="_blank" and add rel="noopener noreferrer" to the external Link.
| </> | ||
| ) : ( | ||
| <div className="h-10 w-40" /> // Placeholder during SSR | ||
| <div className="h-10 w-40" /> |
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.
Consider hiding the SSR placeholder on mobile screens.
The SSR placeholder <div className="h-10 w-40" /> will render on mobile devices (where desktop nav is hidden), potentially causing layout issues before client hydration. The mobile navigation at Line 189 shows null when not mounted, creating inconsistent SSR behavior between mobile and desktop.
Apply this diff to hide the placeholder on mobile:
- <div className="h-10 w-40" />
+ <div className="hidden md:block h-10 w-40" />📝 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="h-10 w-40" /> | |
| <div className="hidden md:block h-10 w-40" /> |
🤖 Prompt for AI Agents
In components/HomeHeader.tsx around line 116, the SSR placeholder div (<div
className="h-10 w-40" />) is rendered on mobile causing inconsistent layout
before hydration; update the element to be hidden on small screens (e.g., add
responsive classes such as hidden md:block or sm:hidden md:block depending on
your breakpoint convention) so the placeholder only appears on desktop where the
desktop nav is shown, keeping SSR output consistent with the mobile null nav.
| {session.user?.isAdmin && ( | ||
| <Link href="/admin"> | ||
| <Button variant="ghost" className="w-full justify-start"> | ||
| Dashboard | ||
| </Button> | ||
| </Link> | ||
| )} |
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.
Duplicate Dashboard link in mobile menu.
Admin users see the Dashboard link twice in the mobile menu:
- Once near the top (Lines 140-146)
- Again in the user profile section (Lines 157-164)
If both are intentional for different navigation paths, consider adding visual separation or labels to clarify their purpose. Otherwise, remove one of the duplicates.
Also applies to: 157-164
🤖 Prompt for AI Agents
In components/HomeHeader.tsx around lines 140-146 and 157-164, there is a
duplicate "Dashboard" link rendered for admin users in the mobile menu; remove
the redundant one (pick one location to keep — e.g., keep the link in the user
profile section at 157-164 and delete the block at 140-146) or alternatively
render only one by moving the isAdmin check so the Dashboard link is
conditionally shown in a single place; ensure any CSS/ARIA expectations remain
correct after removal.
User description
Description
Improved responsiveness across the landing page and header. Fixed layout issues where components appeared left-aligned instead of centered, and adjusted typography and button sizing for all screen widths. Implemented a mobile-first approach with consistent breakpoints and proper container centering.
Type of Change
Related Issue
Closes #
Changes Made
Header Component
Added mobile navigation menu for screens < 768px
Responsive button sizing and improved container centering
Text truncation for user names on smaller screens
Landing Page:
Hero Section: Responsive typography scaling (text-3xl to text-7xl), buttons stack on mobile
Featured Tools & Features: Responsive grid layouts, improved padding and spacing
CTA & Footer: Responsive text sizing and improved mobile layout
Overall: Mobile-first approach with consistent breakpoints and proper container centering across all sections
Screenshots (if applicable)
Tool Information (if adding a new tool)
Testing
Checklist
Additional Context
For Maintainers:
PR Type
Enhancement, Bug fix
Description
Implemented mobile-first responsive design with breakpoints (sm, md, lg, xl)
Added hamburger menu navigation for screens below 768px using Sheet component
Fixed component centering and layout alignment issues across all screen sizes
Responsive typography scaling and button sizing for improved mobile experience
Diagram Walkthrough
File Walkthrough
page.tsx
Responsive layout and typography scalingapp/page.tsx
sm:py-24 lg:py-32)
text-7xl)
width on small screens
with responsive gaps)
HomeHeader.tsx
Mobile hamburger menu and responsive headercomponents/HomeHeader.tsx
breakpoint
profile section
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.