Skip to content

feat(ag-p03): header with nav dropdowns, theme toggle, mobile burger + drawer#856

Merged
ashleyshaw merged 3 commits into
developfrom
feat/ag-p03-header
Jun 6, 2026
Merged

feat(ag-p03): header with nav dropdowns, theme toggle, mobile burger + drawer#856
ashleyshaw merged 3 commits into
developfrom
feat/ag-p03-header

Conversation

@ashleyshaw

@ashleyshaw ashleyshaw commented Jun 6, 2026

Copy link
Copy Markdown
Member

Phase 03 — Header component

AG Build Phase 03 — feat/ag-p03-headerdevelop

Linked issues

Closes #855

Changelog

Added

  • website/src/components/Header.astro — full sticky header with Browse/Resources mega-dropdowns, mobile-actions cluster (theme → search → burger order), right-side slide-in drawer with all spec-required sections (BROWSE, COOK & LEARN, RESOURCES, INSTALL SOURCE, EXTERNAL).
  • website/src/scripts/theme-toggle.js — JS-injected single icon (moon in light mode, sun in dark mode); all .theme-toggle-btn buttons sync simultaneously; uses ag-theme localStorage key.
  • website/src/scripts/header.js — scroll-shrink via IntersectionObserver sentinel (64px → 52px after 60px scroll), dropdown open/close with keyboard support, burger/drawer toggle, branch toggle with ag-branch key, search palette event dispatch, ⌘K/Ctrl+K shortcut.

Changed

  • website/src/layouts/AwesomeGithubLayout.astro — switched from AwesomeGithubNav to the new spec-compliant Header component; removed inline theme-toggle delegation script (now handled by theme-toggle.js).

Fixed

  • Theme toggle was rendering both moon and sun SVGs with CSS display:none toggling (both icons could show simultaneously). Fixed to JS-inject a single icon per the v2 spec.
  • Mobile header was missing the dedicated theme icon button. Added the 3-icon cluster in correct order: theme → search → burger.
  • Branch toggle was using github_branch localStorage key. Corrected to ag-branch per spec.

Risk Assessment

Risk Level: Low

Potential Impact: UI-only change to the site header and its client-side JS. No data, no API, no auth. Worst case is a header that renders incorrectly, which would be immediately visible.

Mitigation Steps:

  • Astro build verified clean (220 pages, zero errors/warnings).
  • JS-injected icon approach eliminates the CSS specificity race that caused the dual-icon bug.
  • is:global style block scopes brand-icon swap rules correctly across component boundaries.
  • Existing AwesomeGithubNav.astro preserved (not deleted) for reference.

How to Test

Prerequisites

  • Node 20+, run npm ci inside website/

Test Steps

  1. Build: npx astro build — expect 220 pages, no errors.
  2. Desktop (>1024px): Verify Browse dropdown opens on click (chevron rotates), closes on outside click and Escape. Verify Resources dropdown same. Verify only one icon visible on theme toggle (moon = light, sun = dark). Verify scroll past 80px shrinks nav to 52px.
  3. Mobile (≤1024px): Resize to 768px. Verify primary nav hidden, 3 icon buttons visible right-aligned (theme, search, burger — in that order). Tap burger → drawer slides from right. Tap X or scrim → drawer closes.
  4. Theme sync: Toggle theme in drawer; verify nav button and mobile button stay in sync (all show same icon).
  5. Branch key: Open DevTools → Application → localStorage; toggle branch → verify key is ag-branch.
  6. No errors: Check browser console — zero errors on load, open, close.

Expected Results

  • Nav is translucent + blurred (not solid white/dark).
  • "GitHub" wordmark is accent-coloured (blue in light, cyan in dark).
  • Brand icon swaps Blue.svg ↔ Light-Blue.svg with theme (no JS flicker).
  • Search pill shows "Search resources" + ⌘K on desktop, collapses to icon on mobile.

Edge Cases to Verify

  • Both themes tested (light and dark)
  • Mobile ≤1024px and desktop >1024px
  • Drawer open → resize to desktop → drawer auto-hides via CSS
  • Keyboard: Tab through nav, Enter opens dropdown, Escape closes, focus returns to trigger
  • No hardcoded colour values outside documented exceptions

Checklist (Global DoD / PR)

  • All AC met and demonstrated (see acceptance checklist in issue [Phase 03] Header: nav, dropdowns, theme toggle, mobile #855)
  • Tests added/updated — build passes cleanly; manual test steps documented above
  • Accessibility checklist completed:
    • Semantic HTML: <header role="banner">, <nav aria-label>, <aside aria-label>, skip link retained
    • Keyboard navigation: dropdowns close on Escape, drawer closes on Escape, focus returns to trigger
    • ARIA used where needed: aria-hidden, aria-expanded, aria-label, aria-pressed, inert
    • Contrast: all surfaces use token system, no hardcoded colours
  • Changelog updated above
  • Frontmatter not applicable (JS/Astro components)
  • Security: no untrusted input; localStorage access is try/caught; no secrets
  • CI build passes (Astro static build, 220 pages)
  • Risk assessment completed above
  • Testing instructions provided above

References

  • Spec: AG_PHASE_03.md, CLAUDE_CODE_HEADER_FOOTER_SPEC.md, CLAUDE_CODE_HEADER_V2_SPEC.md
  • Token audit: AWESOME_GITHUB_THEME_TOKENS_SPEC.md §3.2
  • Orchestrator: AG_ORCHESTRATOR.md

- src/components/Header.astro: full sticky nav, Browse/Resources dropdowns,
  mobile-actions cluster (theme → search → burger), right-side drawer
- src/scripts/theme-toggle.js: JS-injected single icon (moon/sun), all
  .theme-toggle-btn buttons sync; storage key ag-theme
- src/scripts/header.js: scroll-shrink sentinel, dropdown open/close,
  burger/drawer, branch toggle (ag-branch key), search dispatch, ⌘K
- AwesomeGithubLayout.astro: switches from AwesomeGithubNav to Header

Closes #855
@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

Release Notes

  • New Features
    • Added responsive site header with mobile navigation drawer, category dropdowns, and search functionality.
    • Introduced theme toggle for light and dark mode switching with persistent storage.
    • Added keyboard shortcuts (⌘K/Ctrl+K) for search access.
    • Implemented branch selection toggle with persistent preferences.
    • Enhanced navigation with scroll-aware header behaviour and improved accessibility controls.

Walkthrough

This PR replaces the legacy catalogue-driven navigation with a new responsive Header component featuring a sticky desktop navbar with Browse/Resources dropdowns, mobile drawer, theme toggle, search trigger, and branch selector. The layout wiring is updated, and two new client-side scripts handle all interactive behaviours (dropdowns, drawer, search, theme, branch persistence).

Changes

Header Component Refactor

Layer / File(s) Summary
Header component markup and styling
website/src/components/Header.astro
Icon/SVG helpers, desktop sticky navbar with category dropdowns and search, mobile off-canvas drawer with browse grid and resources, global theme-aware brand icon swapping, scroll-lock when drawer is open, and responsive CSS for layout, animations, and visibility at 1024px breakpoints.
Layout integration
website/src/layouts/AwesomeGithubLayout.astro
Removes old AwesomeGithubNav import and catalogue-driven pathname setup; imports and renders new Header component before main content slot and AwesomeGithubFooter.
Header interaction behaviours
website/src/scripts/header.js
Scroll-shrink nav via IntersectionObserver, dropdown open/close with Escape/outside-click, burger-menu drawer with inert and focus management, persistent branch selection via localStorage, and search palette triggering via ⌘K/Ctrl+K plus ag:open-search custom event; updates keyboard hint text for macOS vs. other platforms.
Theme toggle behaviour
website/src/scripts/theme-toggle.js
Reads/writes theme state from data-theme and localStorage (ag-theme), updates toggle button icons and ARIA attributes, sets colorScheme for system-aware rendering, and dispatches theme-changed event on toggle.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • lightspeedwp/.github#845: Overlaps with the new Header.astro UI design and interactive behaviours (mega-dropdown, mobile drawer, branch persistence, theme-changed event, and ag:open-search dispatching).
  • lightspeedwp/.github#849: Moves Browse/Resources dropdown interactions and branch toggle persistence from prior AwesomeGithubNav into the new Header.astro and header.js implementation.
  • lightspeedwp/.github#843: Replaces the previous AwesomeGithubNav mobile menu and theme UI with the new Header.astro drawer and theme-toggle.js functionality.

Suggested labels

area:theme, area:scripts, lang:css

Suggested reviewers

  • krugazul
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly describes the primary change: header component with nav dropdowns, theme toggle, and mobile burger/drawer UI—directly aligned with the main deliverables of Phase 03.
Description check ✅ Passed PR description is comprehensive: linked issues completed, changelog populated across Added/Changed/Fixed sections, risk assessment provided, test steps documented, accessibility/security checklists completed, and all required sections from the template addressed.
Linked Issues check ✅ Passed All Phase 03 deliverables are met: Header.astro, header.js, and theme-toggle.js created; header wired into layout; mobile icons visible; single theme icon (no dual rendering); brand icon swaps via CSS; scroll-shrink at 52px after 60px; tokens comply with spec [#855].
Out of Scope Changes check ✅ Passed All changes are scoped to Phase 03 header objectives. Only Header.astro, header.js, theme-toggle.js, and layout switch are included—no unrelated refactors or dependencies touched.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ag-p03-header

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

website/src/scripts/theme-toggle.js

Parsing error: Unexpected token }


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

✅ Template check passed after update. Thanks for fixing the PR description.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

🔍 Reviewer Summary for PR #856

CI Status:success
Files changed: 0
Risk Distribution: 0 critical, 0 high, 0 medium, 0 low

Recommendations

  • Ready to proceed pending human review

@ashleyshaw ashleyshaw marked this pull request as ready for review June 6, 2026 15:20

Copy link
Copy Markdown
Member Author

@coderabbitai review


Generated by Claude Code

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new, modular Header component along with dedicated scripts for header interactions and theme toggling, replacing the previous navigation layout. The code review feedback highlights several key improvement opportunities: enhancing accessibility by dynamically updating aria-expanded states and trapping keyboard focus inside the mobile drawer; dispatching custom events for branch and theme changes to prevent regressions in other components; replacing the deprecated navigator.platform API; and refactoring duplicated initialization logic in the theme toggle script.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread website/src/scripts/header.js Outdated
Comment on lines +19 to +40
document.querySelectorAll('.nav-dropdown-trigger').forEach(trigger => {
const parent = trigger.closest('.nav-dropdown-wrap');
const dropdown = parent?.querySelector('.nav-dropdown');
if (!parent || !dropdown) return;

trigger.addEventListener('click', e => {
e.stopPropagation();
const isOpen = parent.classList.contains('open');
document.querySelectorAll('.nav-dropdown-wrap.open').forEach(d =>
d.classList.remove('open'));
if (!isOpen) parent.classList.add('open');
});

trigger.addEventListener('keydown', e => {
if (e.key === 'Escape') parent.classList.remove('open');
});
});

document.addEventListener('click', () => {
document.querySelectorAll('.nav-dropdown-wrap.open').forEach(d =>
d.classList.remove('open'));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The dropdown triggers have aria-expanded="false" hardcoded in the HTML, but their expanded state is never updated dynamically in JavaScript when the dropdowns are opened or closed. This prevents screen readers from announcing the correct state of the menus.

Updating aria-expanded dynamically on click, escape key, and outside clicks ensures full accessibility compliance.

  document.querySelectorAll('.nav-dropdown-trigger').forEach(trigger => {
    const parent = trigger.closest('.nav-dropdown-wrap');
    const dropdown = parent?.querySelector('.nav-dropdown');
    if (!parent || !dropdown) return;

    trigger.addEventListener('click', e => {
      e.stopPropagation();
      const isOpen = parent.classList.contains('open');
      document.querySelectorAll('.nav-dropdown-wrap.open').forEach(d => {
        d.classList.remove('open');
        d.querySelector('.nav-dropdown-trigger')?.setAttribute('aria-expanded', 'false');
      });
      if (!isOpen) {
        parent.classList.add('open');
        trigger.setAttribute('aria-expanded', 'true');
      }
    });

    trigger.addEventListener('keydown', e => {
      if (e.key === 'Escape') {
        parent.classList.remove('open');
        trigger.setAttribute('aria-expanded', 'false');
        trigger.focus();
      }
    });
  });

  document.addEventListener('click', () => {
    document.querySelectorAll('.nav-dropdown-wrap.open').forEach(d => {
      d.classList.remove('open');
      d.querySelector('.nav-dropdown-trigger')?.setAttribute('aria-expanded', 'false');
    });
  });

Comment on lines +48 to +59
function openDrawer() {
document.body.classList.add('drawer-open');
drawer?.setAttribute('aria-hidden', 'false');
drawer?.removeAttribute('inert');
drawerClose?.focus();
}
function closeDrawer() {
document.body.classList.remove('drawer-open');
drawer?.setAttribute('aria-hidden', 'true');
drawer?.setAttribute('inert', '');
burger?.focus();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When the mobile navigation drawer is open, keyboard focus is not trapped inside it. Users can still tab into background elements (like header links or main content), which violates accessibility standards for modal dialogs/drawers.

Using the native inert attribute on all other top-level body children when the drawer is open elegantly traps focus inside the drawer.

  function openDrawer() {
    document.body.classList.add('drawer-open');
    drawer?.setAttribute('aria-hidden', 'false');
    drawer?.removeAttribute('inert');
    Array.from(document.body.children).forEach(child => {
      if (child !== drawer && child !== drawerScrim) {
        child.setAttribute('inert', '');
      }
    });
    drawerClose?.focus();
  }
  function closeDrawer() {
    document.body.classList.remove('drawer-open');
    drawer?.setAttribute('aria-hidden', 'true');
    drawer?.setAttribute('inert', '');
    Array.from(document.body.children).forEach(child => {
      if (child !== drawer && child !== drawerScrim) {
        child.removeAttribute('inert');
      }
    });
    burger?.focus();
  }

Comment thread website/src/scripts/header.js Outdated
Comment on lines +82 to +86
btn.addEventListener('click', () => {
try { localStorage.setItem(BRANCH_KEY, b); } catch (e) {}
branchBtns.forEach(x =>
x.setAttribute('aria-pressed', x.dataset.branch === b ? 'true' : 'false'));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When the repository branch is toggled, the active branch is saved to localStorage, but no event is dispatched to notify other components on the page (such as install buttons or clone commands) to update their URLs. Dispatching a custom ag:branch-changed event allows other components to listen and update dynamically without requiring a full page reload.

    btn.addEventListener('click', () => {
      try { localStorage.setItem(BRANCH_KEY, b); } catch (e) {}
      branchBtns.forEach(x =>
        x.setAttribute('aria-pressed', x.dataset.branch === b ? 'true' : 'false'));
      document.dispatchEvent(new CustomEvent('ag:branch-changed', { detail: { branch: b } }));
    });

Comment thread website/src/scripts/header.js Outdated
Comment thread website/src/scripts/theme-toggle.js
Comment thread website/src/scripts/theme-toggle.js Outdated
@github-actions github-actions Bot added status:needs-review Awaiting code review type:feature Feature or enhancement priority:normal Default priority lang:js JavaScript/TypeScript type:chore Chore / small hygiene change meta:needs-changelog Requires a changelog entry before merge labels Jun 6, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ed2408c9c1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +662 to +664
@media (min-width: 1025px) {
#nav-drawer,
#drawer-scrim { display: none; }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear the drawer state when desktop layout takes over

When a user opens the mobile drawer and then rotates or resizes to a desktop viewport, this media query hides the drawer and scrim but nothing calls closeDrawer(). The drawer-open class added by header.js therefore remains on <body>, and the global body.drawer-open { overflow: hidden; } rule keeps the desktop page locked from scrolling until the user returns to mobile and closes the drawer.

Useful? React with 👍 / 👎.

Comment on lines +48 to +52
function openDrawer() {
document.body.classList.add('drawer-open');
drawer?.setAttribute('aria-hidden', 'false');
drawer?.removeAttribute('inert');
drawerClose?.focus();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make the page inert while the mobile drawer is open

When the drawer is open on mobile, this only exposes and focuses the drawer; it does not make the underlying header, main content, or footer inert. Keyboard users can therefore tab past the drawer into controls behind the scrim, which breaks the modal drawer behaviour and the accessibility checklist for inert in this change.

Useful? React with 👍 / 👎.

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details and files changed
gh pr view 856 --repo lightspeedwp/.github --json files,additions,deletions,changedFiles

echo "=== FILES CHANGED ==="
gh pr diff 856 --repo lightspeedwp/.github

Length of output: 31165


🏁 Script executed:

#!/bin/bash
# Check what event name the search palette listens for
rg -n "search-palette-open|ag:open-search" --type astro --type js -C3

# Check AwesomeGithubNav.astro for reference
fd "AwesomeGithubNav.astro" --exec cat {}

Length of output: 26373



@ashleyshaw Happy to review this! 🎉 The component is well-structured overall — the sticky shrink, theme-sync pattern, and drawer anatomy are solid. I did find several regressions vs the old AwesomeGithubNav.astro that are worth addressing before merge, ranging from keyboard/screen-reader breakage to a scroll-lock bug. Here's the full breakdown:


🔴 Critical — Accessibility Regressions

1. aria-expanded is never updated when dropdowns open/close (header.js)

The old nav explicitly toggled aria-expanded on the trigger. The new header.js only adds/removes the .open CSS class on the wrapper, leaving aria-expanded="false" on the button forever — so screen readers never announce the open state.

// header.js — inside the .nav-dropdown-trigger forEach
trigger.addEventListener('click', e => {
  e.stopPropagation();
  const isOpen = parent.classList.contains('open');
  // Close all others
  document.querySelectorAll('.nav-dropdown-wrap.open').forEach(d => {
    d.classList.remove('open');
    d.querySelector('.nav-dropdown-trigger')?.setAttribute('aria-expanded', 'false'); // ← missing
  });
  if (!isOpen) {
    parent.classList.add('open');
    trigger.setAttribute('aria-expanded', 'true'); // ← missing
  }
});

Also, the Escape keydown handler should reset aria-expanded:

trigger.addEventListener('keydown', e => {
  if (e.key === 'Escape') {
    parent.classList.remove('open');
    trigger.setAttribute('aria-expanded', 'false'); // ← missing
    trigger.focus();
  }
});

2. Focus trap missing for mobile drawer (header.js)

The old nav did this on drawer open:

const bgEls = Array.from(
  document.querySelectorAll("body > :not(header):not(aside):not(.drawer-scrim)")
);
bgEls.forEach(el => el.setAttribute("inert", ""));   // open
bgEls.forEach(el => el.removeAttribute("inert"));     // close

header.js never applies inert to background elements, so keyboard users can Tab into <main> content behind the scrim — a WCAG 2.1 (2.1.2) violation.


3. Burger button missing aria-expanded + aria-controls (Header.astro + header.js)

HTML:

<!-- Current — missing both attrs -->
<button class="icon-btn burger" type="button" id="burger-btn" aria-label="Open menu">

<!-- Fix -->
<button class="icon-btn burger" type="button" id="burger-btn"
        aria-label="Open menu"
        aria-expanded="false"
        aria-controls="nav-drawer">

JS:

function openDrawer() {
  burger?.setAttribute('aria-expanded', 'true');  // ← missing
  burger?.setAttribute('aria-label', 'Close menu'); // optional but nice
  ...
}
function closeDrawer() {
  burger?.setAttribute('aria-expanded', 'false'); // ← missing
  burger?.setAttribute('aria-label', 'Open menu');
  ...
}

🟠 Major — Functional Bugs

4. Search event name change may silently break search

Old layout dispatched search-palette-open; new scripts dispatch ag:open-search. If the search palette component (or any other consumer) still listens for search-palette-open, search will silently do nothing. Worth a quick rg 'search-palette-open' across the entire codebase to confirm all listeners are updated.


5. Drawer scroll-lock persists after resize to desktop

The old nav had:

window.matchMedia("(min-width: 1025px)").addEventListener("change", (e) => {
  if (e.matches) closeDrawer();
});

header.js has no equivalent. If a user opens the drawer then resizes to desktop, body.drawer-open stays, keeping overflow: hidden on the body (scroll locked) even though the drawer is CSS-hidden. Fix:

window.matchMedia("(min-width: 1025px)").addEventListener("change", (e) => {
  if (e.matches && document.body.classList.contains('drawer-open')) closeDrawer();
});

6. aria-current="page" removed from nav links

The old nav had:

<a href={`${base}cookbook/`} class="nav-btn"
   aria-current={isActive("/cookbook") ? "page" : undefined}>

The new Header.astro omits aria-current on all links. Screen reader users lose the active page indication. Consider reinstating Astro.url / import.meta.env.BASE_URL based active-link detection.


🟡 Minor

7. navigator.platform is deprecated (header.js, Line 103)

// Deprecated
const isMac = navigator.platform.toUpperCase().includes('MAC');

// Modern with fallback
const isMac = (navigator.userAgentData?.platform ?? navigator.platform).toUpperCase().includes('MAC');

8. Dropdown focusout close behaviour removed

The old nav closed dropdowns when keyboard focus moved out of the wrapper via a focusout handler. The new implementation has no equivalent, so Tab-navigating out of an open dropdown leaves it open. Consider adding:

parent.addEventListener('focusout', e => {
  if (!parent.contains(e.relatedTarget)) {
    parent.classList.remove('open');
    trigger.setAttribute('aria-expanded', 'false');
  }
});

Summary table

# Severity File Issue
1 🔴 Critical header.js aria-expanded never toggled on dropdown triggers
2 🔴 Critical header.js No inert on background elements when drawer opens
3 🔴 Critical Header.astro + header.js Burger missing aria-expanded / aria-controls
4 🟠 Major header.js + Header.astro Search event name change (search-palette-openag:open-search) — verify all consumers updated
5 🟠 Major header.js No matchMedia listener — scroll lock persists after resize
6 🟠 Major Header.astro aria-current="page" removed from nav links
7 🟡 Minor header.js navigator.platform deprecated
8 🟡 Minor header.js Dropdown focusout close behaviour not ported

Items 1–3 are regressions from the old nav and block WCAG compliance. Happy to help draft diffs for any of these if useful! 🙌

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements the Phase 03 Awesome GitHub site header, replacing the previous nav with a new Header.astro component and associated client-side behaviour scripts.

Changes:

  • Added a new header component with desktop dropdown navigation, theme toggle buttons, and a mobile burger + drawer.
  • Introduced client-side scripts for theme icon swapping and header interactions (dropdowns, drawer, scroll-shrink, branch toggle, search trigger wiring).
  • Wired the new header into AwesomeGithubLayout.astro, removing the legacy inline theme/search wiring.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File Description
website/src/components/Header.astro New header markup, global and scoped styles, plus script imports for header behaviour.
website/src/scripts/theme-toggle.js Theme toggle behaviour with single-icon rendering across all toggle buttons.
website/src/scripts/header.js Dropdown, drawer, scroll-shrink, branch toggle, and search trigger/keyboard shortcut wiring.
website/src/layouts/AwesomeGithubLayout.astro Swaps legacy nav usage for the new Header component and removes inline handlers.

Comment thread website/src/scripts/header.js Outdated
Comment thread website/src/scripts/header.js Outdated
Comment thread website/src/components/Header.astro Outdated
Comment thread website/src/scripts/header.js
Comment thread website/src/scripts/theme-toggle.js
Comment thread website/src/scripts/header.js Outdated
Comment thread website/src/scripts/header.js Outdated
Comment on lines +106 to +109
const isMac = navigator.platform.toUpperCase().includes('MAC');
document.querySelectorAll('.search-kbd-hint').forEach(el => {
el.textContent = isMac ? '⌘K' : 'Ctrl K';
});
Comment thread website/src/components/Header.astro
Comment thread website/src/components/Header.astro
Comment thread website/src/components/Header.astro
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Ash Shaw <ashley@lightspeedwp.agency>
@github-actions github-actions Bot removed the type:chore Chore / small hygiene change label Jun 6, 2026
@coderabbitai coderabbitai Bot requested a review from krugazul June 6, 2026 15:35
@coderabbitai coderabbitai Bot added area:theme Theme & styles lang:css Stylesheets (CSS/Sass/etc.) area:scripts Scripts & tooling labels Jun 6, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@website/src/components/Header.astro`:
- Around line 266-267: The header's z-index (z-index: 100) is higher than the
drawer/scrim (90/91), leaving header controls clickable while the drawer is
open; fix by making the drawer/scrim stack above the header (e.g., increase the
scrim/drawer z-index to >100) or lower the header z-index below the scrim (e.g.,
set z-index: 50 on the header). Update the CSS rule containing "z-index: 100" in
Header.astro or the CSS rules that set the scrim/drawer z-index (the rules using
90/91) so the drawer's z-index is greater than the header, and ensure their
positioning/stacking contexts remain consistent.

In `@website/src/scripts/header.js`:
- Around line 87-98: The persisted branch key currently uses BRANCH_KEY =
'ag-branch' which mismatches the downstream reader expecting 'github_branch';
update BRANCH_KEY to 'github_branch' (and ensure any references to 'ag-branch'
in header.js are replaced) so the toggle (branchBtns logic that sets
aria-pressed and calls localStorage.setItem) and the page reader (github_branch)
use the same storage key.
- Around line 59-74: openDrawer/closeDrawer currently only toggle inert on the
drawer element, allowing background elements to remain tabbable and receive
focus; update these functions (openDrawer, closeDrawer) to set inert (and
aria-hidden where appropriate) on all page elements except the drawer and its
controls when opening, and remove inert from those elements when closing so
background content is fully excluded from focus and interaction; identify
targets by querying top-level children (e.g., document.body.children or a
container like main/header/footer) and skip the drawer, burger, and drawerClose
elements when toggling inert/aria-hidden, and ensure focus is moved into the
drawer on open and returned to the triggering control on close.
- Around line 80-84: The drawer can remain logically open after a viewport
crosses the desktop breakpoint because we only close it on Escape; add a
resize/viewport-change handler (e.g., use window.matchMedia and its change event
or window.addEventListener('resize')) that detects when the viewport becomes
desktop-sized and then calls the same cleanup used by closeDrawer (remove
document.body.classList.contains('drawer-open') state, remove the 'drawer-open'
class, clear scroll-lock and scrim, and restore any ARIA attributes). Locate the
existing document.addEventListener('keydown', ...) and reuse the same
closeDrawer cleanup logic to ensure all state (body class, ARIA, scroll
restoration) is cleared on breakpoint transition.
- Around line 41-43: The Escape key handler removes the visual "open" class but
doesn't update the ARIA state, causing assistive tech to still report the
dropdown as open; update the keydown listener on trigger so that after
parent.classList.remove('open') you also set the controlling element's
aria-expanded to "false" (e.g., call trigger.setAttribute('aria-expanded',
'false') or update the same attribute used elsewhere), ensuring the visible
state and ARIA state stay in sync.

In `@website/src/scripts/theme-toggle.js`:
- Around line 22-31: There is a stray closing brace after the updateAllIcons()
function causing a JS syntax error; open the block containing function
updateAllIcons (which uses getTheme, SVG_SUN and SVG_MOON) and remove the extra
`}` so the function and surrounding scope close only once, ensuring no unmatched
braces remain and the file ends with the correct closure.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: c2ec9ff2-8bae-46be-806b-b3fa649b14ae

📥 Commits

Reviewing files that changed from the base of the PR and between 2eaf8a8 and 6dcb2ac.

📒 Files selected for processing (4)
  • website/src/components/Header.astro
  • website/src/layouts/AwesomeGithubLayout.astro
  • website/src/scripts/header.js
  • website/src/scripts/theme-toggle.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{php,js,jsx,ts,tsx,css,scss,html}

📄 CodeRabbit inference engine (AGENTS.md)

Follow WordPress Coding Standards (CSS, HTML, JavaScript, PHP) and inline-documentation standards at all times

Files:

  • website/src/scripts/header.js
  • website/src/scripts/theme-toggle.js
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{js,ts,jsx,tsx}: Follow ESLint/Prettier standards for JavaScript and TypeScript files
Avoid unnecessary JavaScript, defer/lazy-load where possible, and prefer native blocks

Files:

  • website/src/scripts/header.js
  • website/src/scripts/theme-toggle.js
**/*.{js,ts}

⚙️ CodeRabbit configuration file

**/*.{js,ts}: Review JavaScript/TypeScript:

  • Ensure code is linted and follows project style guides.
  • Check for dead code, unused variables, and clear function naming.
  • Validate accessibility and performance optimisations.
  • Ensure tests are isolated and do not depend on external state.
  • Check for descriptive test names and clear test structure.

Files:

  • website/src/scripts/header.js
  • website/src/scripts/theme-toggle.js
🪛 ESLint
website/src/scripts/header.js

[error] 3-3: 'document' is not defined.

(no-undef)


[error] 6-6: 'document' is not defined.

(no-undef)


[error] 7-7: 'window' is not defined.

(no-undef)


[error] 8-8: 'document' is not defined.

(no-undef)


[error] 10-10: 'document' is not defined.

(no-undef)


[error] 11-11: 'IntersectionObserver' is not defined.

(no-undef)


[error] 19-19: 'document' is not defined.

(no-undef)


[error] 28-28: 'document' is not defined.

(no-undef)


[error] 46-46: 'document' is not defined.

(no-undef)


[error] 47-47: 'document' is not defined.

(no-undef)


[error] 54-54: 'document' is not defined.

(no-undef)


[error] 55-55: 'document' is not defined.

(no-undef)


[error] 56-56: 'document' is not defined.

(no-undef)


[error] 57-57: 'document' is not defined.

(no-undef)


[error] 60-60: 'document' is not defined.

(no-undef)


[error] 68-68: 'document' is not defined.

(no-undef)


[error] 80-80: 'document' is not defined.

(no-undef)


[error] 81-81: 'document' is not defined.

(no-undef)


[error] 88-88: 'document' is not defined.

(no-undef)


[error] 91-91: 'localStorage' is not defined.

(no-undef)


[error] 98-98: 'localStorage' is not defined.

(no-undef)


[error] 98-98: Empty block statement.

(no-empty)


[error] 105-105: 'document' is not defined.

(no-undef)


[error] 106-106: 'document' is not defined.

(no-undef)


[error] 106-106: 'CustomEvent' is not defined.

(no-undef)


[error] 108-108: 'document' is not defined.

(no-undef)


[error] 109-109: 'document' is not defined.

(no-undef)


[error] 109-109: 'CustomEvent' is not defined.

(no-undef)


[error] 113-113: 'document' is not defined.

(no-undef)


[error] 116-116: 'document' is not defined.

(no-undef)


[error] 116-116: 'CustomEvent' is not defined.

(no-undef)


[error] 121-121: 'navigator' is not defined.

(no-undef)


[error] 122-122: 'document' is not defined.

(no-undef)

🪛 GitHub Actions: Awesome GitHub Site / 1_Build website.txt
website/src/scripts/theme-toggle.js

[error] 30-30: Vite/Rollup build failed: Expression expected. Error location: src/scripts/theme-toggle.js:30:0.

🪛 GitHub Actions: Awesome GitHub Site / Build website
website/src/scripts/theme-toggle.js

[error] 30-30: Build failed in Vite/Rollup: Expression expected at src/scripts/theme-toggle.js:30:0.

🔇 Additional comments (1)
website/src/layouts/AwesomeGithubLayout.astro (1)

4-5: LGTM!

Also applies to: 29-31

Comment on lines +266 to +267
z-index: 100;
height: var(--nav-h, 64px);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Drawer stack order is upside-down (header stays clickable above the modal).

At Line 266, the header sits at z-index: 100, while the scrim/drawer at Lines 551 and 570 are lower (90/91). Result: background header controls stay interactive while the drawer is open, which breaks modal isolation.

Suggested fix
-  .nav {
+  .nav {
     position: sticky;
     top: 0;
-    z-index: 100;
+    z-index: 80;
     height: var(--nav-h, 64px);
     ...
   }

   `#drawer-scrim` {
     position: fixed;
     inset: 0;
-    z-index: 90;
+    z-index: 110;
     ...
   }

   `#nav-drawer` {
     position: fixed;
     ...
-    z-index: 91;
+    z-index: 111;
     ...
   }

Also applies to: 551-552, 570-571

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@website/src/components/Header.astro` around lines 266 - 267, The header's
z-index (z-index: 100) is higher than the drawer/scrim (90/91), leaving header
controls clickable while the drawer is open; fix by making the drawer/scrim
stack above the header (e.g., increase the scrim/drawer z-index to >100) or
lower the header z-index below the scrim (e.g., set z-index: 50 on the header).
Update the CSS rule containing "z-index: 100" in Header.astro or the CSS rules
that set the scrim/drawer z-index (the rules using 90/91) so the drawer's
z-index is greater than the header, and ensure their positioning/stacking
contexts remain consistent.

Comment thread website/src/scripts/header.js Outdated
Comment thread website/src/scripts/header.js
Comment thread website/src/scripts/header.js Outdated
Comment thread website/src/scripts/header.js Outdated
Comment on lines +87 to +98
const BRANCH_KEY = 'ag-branch';
const branchBtns = document.querySelectorAll('.branch-btn');

const savedBranch = (() => {
try { return localStorage.getItem(BRANCH_KEY) || 'main'; } catch (e) { return 'main'; }
})();

branchBtns.forEach(btn => {
const b = btn.dataset.branch;
btn.setAttribute('aria-pressed', b === savedBranch ? 'true' : 'false');
btn.addEventListener('click', () => {
try { localStorage.setItem(BRANCH_KEY, b); } catch (e) {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Branch persistence key breaks the existing cross-file contract.

At Line 87 you persist to ag-branch, but website/src/pages/c/[type]/[slug].astro (Lines 252-277 in provided context) reads github_branch. The header toggle and downstream page branch logic won’t stay in sync.

🧰 Tools
🪛 ESLint

[error] 88-88: 'document' is not defined.

(no-undef)


[error] 91-91: 'localStorage' is not defined.

(no-undef)


[error] 98-98: 'localStorage' is not defined.

(no-undef)


[error] 98-98: Empty block statement.

(no-empty)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@website/src/scripts/header.js` around lines 87 - 98, The persisted branch key
currently uses BRANCH_KEY = 'ag-branch' which mismatches the downstream reader
expecting 'github_branch'; update BRANCH_KEY to 'github_branch' (and ensure any
references to 'ag-branch' in header.js are replaced) so the toggle (branchBtns
logic that sets aria-pressed and calls localStorage.setItem) and the page reader
(github_branch) use the same storage key.

Comment thread website/src/scripts/theme-toggle.js
# Conflicts:
#	website/src/components/Header.astro
#	website/src/scripts/header.js
#	website/src/scripts/theme-toggle.js
@ashleyshaw ashleyshaw merged commit 321b004 into develop Jun 6, 2026
13 of 15 checks passed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

@ashleyshaw ashleyshaw deleted the feat/ag-p03-header branch June 6, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scripts Scripts & tooling area:theme Theme & styles lang:css Stylesheets (CSS/Sass/etc.) lang:js JavaScript/TypeScript meta:needs-changelog Requires a changelog entry before merge priority:normal Default priority status:needs-review Awaiting code review type:feature Feature or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Phase 03] Header: nav, dropdowns, theme toggle, mobile

3 participants