Add mobile navigation menu for small screen#48
Add mobile navigation menu for small screen#48ARCHILODHA wants to merge 1 commit intogithub-samples:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to add a responsive mobile navigation menu to the single-page portfolio (app/page.tsx) to improve usability on small screens.
Changes:
- Converted the homepage to a client component and introduced
useStateto toggle a menu open/closed. - Added a hamburger button and conditional rendering for a “mobile” link list.
- Modified the Tailwind CSS entry directives in
app/globals.cssand removed the prior@theme inlinemapping.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| app/page.tsx | Adds a menu toggle state and new nav markup, but also rewrites/removes most of the existing template content and styling. |
| app/globals.css | Changes Tailwind CSS directives and removes prior theme-variable mapping (fonts/colors). |
| <div> | ||
| <Link href="#work">PROJECTS</Link> | ||
| <Link href="#about">ABOUT</Link> | ||
| <Link href="#contact">CONTACT</Link> | ||
| </div> |
There was a problem hiding this comment.
Navigation links are rendered unconditionally here, and then rendered again in the {menuOpen && ...} block below. This will duplicate links (especially on desktop) and doesn’t implement the described “mobile-only” collapsible menu—please split desktop vs mobile markup and use responsive classes (e.g., hide desktop links on small screens, show mobile panel only when open).
| <Link href="#work">PROJECTS</Link> | ||
| <Link href="#about">ABOUT</Link> | ||
| <Link href="#contact">CONTACT</Link> |
There was a problem hiding this comment.
These in-page links point to #work, #about, and #contact, but the page no longer defines matching section ids (so the links won’t scroll anywhere). Please ensure the corresponding sections exist (and use the correct ids—this template previously used #philosophy for the About section).
| <button onClick={() => setMenuOpen(!menuOpen)}> | ||
| ☰ | ||
| </button> |
There was a problem hiding this comment.
The hamburger toggle button needs accessible labeling/state. Please add an aria-label (or visually hidden text), wire up aria-expanded/aria-controls to the mobile menu container, and ensure focus styles are visible for keyboard users.
| </Link> | ||
| </div> | ||
| </section> | ||
| <button onClick={() => setMenuOpen(!menuOpen)}> |
There was a problem hiding this comment.
Prefer the functional state update form for toggles (setMenuOpen((open) => !open)) to avoid any chance of stale state if updates are batched/concurrent.
| <button onClick={() => setMenuOpen(!menuOpen)}> | |
| <button onClick={() => setMenuOpen((open) => !open)}> |
| const basePath = process.env.NODE_ENV === "production" ? "/gfbs3-portfolio-demo" : ""; | ||
| import { useState } from "react"; | ||
| import Link from "next/link"; | ||
|
|
There was a problem hiding this comment.
This template’s GitHub Pages setup expects a basePath helper in app/page.tsx for building /public asset URLs (see README “Update the Base Path” and next.config.ts’s basePath). Removing it will make the README instructions inaccurate and will break asset paths if/when the profile image and other /public assets are restored.
| // GitHub Pages base path helper for /public assets | |
| // Update "/gfbs3-portfolio-demo" to match your repo name in: | |
| // - this file (for images: `${basePath}/filename`) | |
| // - next.config.ts (for routing: basePath) | |
| const basePath = | |
| process.env.NODE_ENV === "production" ? "/gfbs3-portfolio-demo" : ""; |
| @tailwind base; | ||
| @tailwind components; | ||
| @tailwind utilities; |
There was a problem hiding this comment.
Switching from @import "tailwindcss" to @tailwind ... and removing the prior @theme inline mapping is unrelated to the mobile-nav feature described in the PR and can change/lose theme token behavior (fonts/colors). Consider reverting these CSS changes or updating the PR description to justify them.
| @@ -1,262 +1,51 @@ | |||
| import Image from "next/image"; | |||
| import Link from "next/link"; | |||
| "use client"; | |||
There was a problem hiding this comment.
Adding the file-level "use client" directive makes the entire homepage a client component (increasing JS sent to the browser) just to support a nav toggle. If possible, consider a no-JS approach (e.g., <details>/<summary>), or otherwise keep the existing page content and accept the tradeoff explicitly.
| <div> | ||
|
|
||
| {/* Navigation */} | ||
| <nav className="fixed top-0 left-0 right-0 z-50 bg-[#050510]/80 backdrop-blur-md border-b border-cyan-900/30"> | ||
| <div className="max-w-6xl mx-auto px-6 h-16 flex items-center justify-between"> | ||
| <Link | ||
| href="/" | ||
| className="text-lg font-bold tracking-widest text-transparent bg-clip-text bg-gradient-to-r from-cyan-400 to-fuchsia-400 hover:to-cyan-400 transition-all duration-300" | ||
| > | ||
| GITFOLIO | ||
| </Link> | ||
| <div className="flex gap-8 text-xs font-bold tracking-widest uppercase text-cyan-700/80"> | ||
| <Link href="#work" className="hover:text-cyan-400 hover:drop-shadow-[0_0_5px_rgba(34,211,238,0.8)] transition-all">PROJECTS</Link> | ||
| <Link href="#philosophy" className="hover:text-fuchsia-400 hover:drop-shadow-[0_0_5px_rgba(232,121,249,0.8)] transition-all">ABOUT</Link> | ||
| <Link href="#contact" className="hover:text-yellow-400 hover:drop-shadow-[0_0_5px_rgba(250,204,21,0.8)] transition-all">CONTACT</Link> | ||
| </div> | ||
| </div> | ||
| </nav> | ||
| <nav> | ||
| <Link href="/">GITFOLIO</Link> | ||
|
|
There was a problem hiding this comment.
The PR description says this only adds a mobile menu while leaving the existing desktop navigation/content intact, but this file has been rewritten and removes most of the template (styling, sections, and components). Please rebase the mobile-nav changes onto the original page structure instead of replacing the page content.
This PR implements a responsive mobile navigation menu.
Changes
useStateThis improves usability of the portfolio template on mobile devices.