fix(ag-p03): address open review issues — ARIA regressions, focus trap, resize lock, branch event, aria-current#857
Conversation
- 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
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>
…e, branch event, aria-current - theme-toggle.js: remove stray closing brace (ESLint parse error) - header.js: reset aria-expanded + focus trigger on Escape key in dropdowns - header.js: add focusout handler to close dropdown when focus leaves wrapper - header.js: apply inert to background elements when drawer opens (WCAG 2.1.2) - header.js: add matchMedia listener to call closeDrawer() on resize >=1025px - header.js: dispatch ag:branch-changed CustomEvent on branch toggle - Header.astro: add isActive() helper and aria-current='page' on Cookbook/Learn links - eslint.config.cjs: add browser globals config block for website/src/scripts/
…ase-aware isActive - header.js: use child.id comparisons in focus trap rather than object identity - Header.astro: strip BASE_URL prefix from pathname before isActive comparison so active link detection works under any deployment subpath
|
✅ Template check passed after update. Thanks for fixing the PR description. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38657358ba
ℹ️ 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".
| }); | ||
|
|
||
| /* ── Branch toggle ──────────────────────────── */ | ||
| const BRANCH_KEY = "ag-branch"; |
There was a problem hiding this comment.
The new header branch control persists ag-branch, but the catalogue detail page that updates the VS Code/raw/GitHub install links still reads and writes github_branch (website/src/pages/c/[type]/[slug].astro lines 254 and 275), and there is no listener for ag:branch-changed. In practice, selecting develop in the global header before opening an item detail page is ignored, so those install links remain on main until the separate per-page branch toggle is used.
Useful? React with 👍 / 👎.
| trigger.addEventListener("keydown", (e) => { | ||
| if (e.key === "Escape") { | ||
| parent.classList.remove("open"); | ||
| trigger.setAttribute("aria-expanded", "false"); | ||
| trigger.focus(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Escape is only handled on the dropdown trigger, so once keyboard focus has moved into a menu item, pressing Escape leaves the dropdown open and aria-expanded stuck on true. I checked the header dropdown wiring in this file: the only document-level Escape handler is for the mobile drawer, so this path is not covered for the Browse/Resources menu contents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates the Awesome GitHub website header implementation to address previously raised accessibility and functional issues, moving header behaviours into dedicated browser scripts and aligning linting with browser globals.
Changes:
- Added a new
HeaderAstro component and corresponding client-side scripts for dropdowns, drawer, branch toggle, search trigger wiring, and theme toggle icon sync. - Updated the main layout to use the new header and removed the previous inline event-delegation script.
- Extended ESLint flat config to treat
website/src/scripts/**/*.jsas browser code with appropriate globals.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| website/src/scripts/theme-toggle.js | Implements theme toggle behaviour and icon/ARIA sync across all toggle buttons. |
| website/src/scripts/header.js | Implements header interactions (dropdowns, drawer, branch toggle, search wiring, scroll shrink). |
| website/src/layouts/AwesomeGithubLayout.astro | Switches layout header to the new Header component and removes inline JS delegation. |
| website/src/components/Header.astro | Introduces the new header markup, styles, and module imports for browser scripts. |
| eslint.config.cjs | Adds a browser-script ESLint config block for website/src/scripts/**/*.js. |
| burger?.addEventListener("click", openDrawer); | ||
| drawerClose?.addEventListener("click", closeDrawer); | ||
| drawerScrim?.addEventListener("click", closeDrawer); |
| function openDrawer() { | ||
| document.body.classList.add("drawer-open"); | ||
| burger?.setAttribute("aria-expanded", "true"); | ||
| burger?.setAttribute("aria-label", "Close menu"); | ||
| drawer?.setAttribute("aria-hidden", "false"); | ||
| drawer?.removeAttribute("inert"); |
| document.body.classList.remove("drawer-open"); | ||
| burger?.setAttribute("aria-expanded", "false"); | ||
| burger?.setAttribute("aria-label", "Open menu"); | ||
| drawer?.setAttribute("aria-hidden", "true"); | ||
| drawer?.setAttribute("inert", ""); |
| trigger.addEventListener("keydown", (e) => { | ||
| if (e.key === "Escape") { | ||
| parent.classList.remove("open"); | ||
| trigger.setAttribute("aria-expanded", "false"); | ||
| trigger.focus(); | ||
| } | ||
| }); |
| /* ── Search palette wiring ──────────────────── */ | ||
| document | ||
| .getElementById("search-btn-mobile") | ||
| ?.addEventListener("click", () => { | ||
| document.dispatchEvent(new CustomEvent("ag:open-search")); | ||
| }); | ||
| document.querySelector(".search-trigger")?.addEventListener("click", () => { | ||
| document.dispatchEvent(new CustomEvent("ag:open-search")); | ||
| }); |
| /* Wire drawer search button to same event */ | ||
| document.addEventListener('DOMContentLoaded', () => { | ||
| document.getElementById('search-btn-drawer')?.addEventListener('click', () => { | ||
| document.dispatchEvent(new CustomEvent('ag:open-search')); | ||
| }); |
| try { | ||
| localStorage.setItem("ag-theme", next); | ||
| } catch (e) {} |
| try { | ||
| return localStorage.getItem(BRANCH_KEY) || "main"; | ||
| } catch (e) { | ||
| return "main"; | ||
| } |
| try { | ||
| localStorage.setItem(BRANCH_KEY, b); | ||
| } catch (e) {} |
ashleyshaw
left a comment
ashleyshaw
left a comment
There was a problem hiding this comment.
Reviewed the review-fix branch. The syntax, ARIA, focus, inert, resize, and active-link changes line up with the reported issues.
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-btnbuttons sync simultaneously; usesag-themelocalStorage 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 withag-branchkey, search palette event dispatch, Cmd/Ctrl+K shortcut.website/src/scripts/header.js— accessibility and behaviour fixes for Escape handling, focus trap, inert background locking, resize cleanup, and branch-change events.eslint.config.cjs— browser globals block forwebsite/src/scripts/**/*.js.Changed
website/src/layouts/AwesomeGithubLayout.astro— switched fromAwesomeGithubNavto the new spec-compliantHeadercomponent; removed inline theme-toggle delegation script (now handled bytheme-toggle.js).Fixed
display:nonetoggling. Fixed to JS-inject a single icon per the v2 spec.github_branchlocalStorage key. Corrected toag-branchper spec.theme-toggle.jsthat caused a syntax error and build failure.aria-expandedand returns focus to the trigger.aria-current="page"to active Cookbook and Learn links and made active-link detection base-path aware.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:
How to Test
Prerequisites
npm ciinsidewebsite/Test Steps
npx astro build- expect a clean build.Expected Results
Edge Cases to Verify
Checklist (Global DoD / PR)
last_updatedandversion)