-
Notifications
You must be signed in to change notification settings - Fork 10
Add mobile navigation menu for small screen #48
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,262 +1,51 @@ | ||||||||||||||||||
| import Image from "next/image"; | ||||||||||||||||||
| import Link from "next/link"; | ||||||||||||||||||
| "use client"; | ||||||||||||||||||
|
||||||||||||||||||
|
|
||||||||||||||||||
| const basePath = process.env.NODE_ENV === "production" ? "/gfbs3-portfolio-demo" : ""; | ||||||||||||||||||
| import { useState } from "react"; | ||||||||||||||||||
| import Link from "next/link"; | ||||||||||||||||||
|
|
||||||||||||||||||
|
||||||||||||||||||
| // 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" : ""; |
Copilot
AI
Mar 6, 2026
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.
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.
Copilot
AI
Mar 6, 2026
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.
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)}> |
Copilot
AI
Mar 6, 2026
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.
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.
Copilot
AI
Mar 6, 2026
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.
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).
Copilot
AI
Mar 6, 2026
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.
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).
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.
Switching from
@import "tailwindcss"to@tailwind ...and removing the prior@theme inlinemapping 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.