Fix mobile menu display#255
Open
Gijsreyn wants to merge 2 commits into
Open
Conversation
WalkthroughReplaces the transform-based ChangesMobile Navigation Overlay System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@assets/css/futuristic.css`:
- Around line 83-85: The CSS custom property --font-sans has unquoted
font-family keywords (e.g., BlinkMacSystemFont and Roboto) causing Stylelint
value-keyword-case errors; fix it by quoting any multi-word or capitalized font
family names and ensuring consistent quotes for the list used in --font-sans
(e.g., wrap BlinkMacSystemFont and Roboto in quotes or use consistent quoting
for all family names) so Stylelint passes when validating the font-family token.
In `@assets/js/main.js`:
- Around line 105-125: The menu button label isn't updated when toggling the
menu; update the button text/label in both toggleMobileMenu() and
closeMobileMenu() so screen readers see current intent: after toggling open in
toggleMobileMenu() set the menuToggle element's visible label (e.g., textContent
or aria-label) to "Close menu" when isOpen is true and "Open menu" when false,
and in closeMobileMenu() explicitly set menuToggle's label to "Open menu" (or
update aria-label/textContent consistently) while still updating aria-expanded;
reference the elements with IDs "mobileMenu" and "menuToggle" inside those
functions.
In `@layouts/partials/header-new.html`:
- Around line 173-178: Add an aria-controls attribute to the mobile menu toggle
so assistive tech knows which element it controls: update the button with id
"menuToggle" (which calls toggleMobileMenu()) to include
aria-controls="mobileMenu" that matches the mobile menu element's id (e.g., the
menu dialog element should have id="mobileMenu").
- Around line 176-177: The header template contains hardcoded English UI strings
(e.g., the mobile menu button aria-label "Open menu" and other new menu
labels/buttons referenced around the same region) that must be replaced with
i18n keys; update the elements that currently use literal text and aria
attributes (refer to the mobile menu button and the labels introduced near the
other noted blocks) to call the project's localization helper (use the same
i18n/key pattern used elsewhere in the project) and add corresponding string
keys to the locale files so all locales render correctly and no orphaned keys
remain.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 932a18c3-6824-4c14-aaeb-6eec7cff8493
📒 Files selected for processing (3)
assets/css/futuristic.cssassets/js/main.jslayouts/partials/header-new.html
There was a problem hiding this comment.
♻️ Duplicate comments (1)
layouts/partials/header-new.html (1)
173-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
aria-controls="mobileMenu"to complete the accessibility binding.The past review comment requesting
aria-controls="mobileMenu"on this button remains unaddressed. Adding it explicitly binds the toggle to the controlled element for assistive technology.♿ Proposed addition
<button class="menu-toggle" id="menuToggle" aria-label="{{ i18n "mobileMenuOpenAriaLabel" }}" aria-expanded="false" + aria-controls="mobileMenu" onclick="toggleMobileMenu()" >🤖 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 `@layouts/partials/header-new.html` around lines 173 - 178, The menu toggle button lacks an explicit aria-controls binding; add aria-controls="mobileMenu" to the element with id "menuToggle" (the button that calls toggleMobileMenu()) so assistive tech knows it controls the element with id "mobileMenu".
🧹 Nitpick comments (2)
layouts/partials/header-new.html (2)
202-202: 💤 Low valueConsider using
<div role="dialog">for semantic clarity.The
<aside>element is semantically meant for tangentially related content. For a modal dialog overlay,<div role="dialog">or the native<dialog>element is more appropriate.🎨 Semantic alternative
-<aside class="mobile-menu-panel" role="dialog" aria-modal="true" aria-label="{{ i18n "mobileNavigationAriaLabel" }}"> +<div class="mobile-menu-panel" role="dialog" aria-modal="true" aria-label="{{ i18n "mobileNavigationAriaLabel" }}"> ... -</aside> +</div>🤖 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 `@layouts/partials/header-new.html` at line 202, The mobile overlay currently uses an <aside class="mobile-menu-panel"> which is semantically for tangential content; change this element to either a <div role="dialog"> or a native <dialog> to reflect its modal behavior (preserve class="mobile-menu-panel" and all aria attributes like aria-modal and aria-label/i18n). Update any JS/CSS that target the aside selector (e.g., queries or styles referencing aside.mobile-menu-panel) to the new selector (div.mobile-menu-panel or dialog.mobile-menu-panel) and ensure focus management/aria attributes remain unchanged on the element used by the mobile menu code.
59-71: ⚡ Quick winExtract icon selection logic to a Hugo partial to eliminate triplication.
The icon-mapping logic for menu items (
Community→ri-team-line,Resource Modules→ri-apps-2-line, etc.) is repeated three times: desktop dropdown (lines 59–71), mobile submenu (lines 226–238), and mobile top-level items (lines 249–258). Centralizing this in a partial (e.g.,layouts/partials/menu-icon.html) reduces maintenance cost and keeps mappings consistent.♻️ Refactor direction
Create
layouts/partials/menu-icon.html:{{- $name := . -}} {{- if eq $name "Community" -}} <i class="ri-team-line"></i> {{- else if eq $name "Resource Modules" -}} <i class="ri-apps-2-line"></i> {{- else if eq $name "Guidelines" -}} <i class="ri-book-open-line"></i> {{- else if eq $name "Blog Posts" -}} <i class="ri-article-line"></i> {{- else if eq $name "Config. Management" -}} <i class="ri-settings-3-line"></i> {{- else if eq $name "Code of Conduct" -}} <i class="ri-shield-check-line"></i> {{- else if eq $name "Community Calls" -}} <i class="ri-calendar-event-line"></i> {{- else if eq $name "FAQ" -}} <i class="ri-question-answer-line"></i> {{- else if eq $name "Help" -}} <i class="ri-lifebuoy-line"></i> {{- else -}} <i class="ri-file-text-line"></i> {{- end -}}Then replace each block with
{{ partial "menu-icon.html" .Name }}.Also applies to: 226-238, 249-258
🤖 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 `@layouts/partials/header-new.html` around lines 59 - 71, Create a new Hugo partial named menu-icon.html that accepts the menu item name (use the passed-in .Name) and centralizes the mapping of names to icon markup (Community→ri-team-line, Resource Modules→ri-apps-2-line, Guidelines→ri-book-open-line, Blog Posts→ri-article-line, Config. Management→ri-settings-3-line, Code of Conduct→ri-shield-check-line, Community Calls→ri-calendar-event-line, FAQ→ri-question-answer-line, Help→ri-lifebuoy-line, default→ri-file-text-line); then replace each duplicated icon block in header-new.html (the desktop dropdown block using .Name, the mobile submenu block, and the mobile top-level items) with a call to the partial: pass .Name into partial "menu-icon.html" so all three places use the single source of truth for icons.
🤖 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.
Duplicate comments:
In `@layouts/partials/header-new.html`:
- Around line 173-178: The menu toggle button lacks an explicit aria-controls
binding; add aria-controls="mobileMenu" to the element with id "menuToggle" (the
button that calls toggleMobileMenu()) so assistive tech knows it controls the
element with id "mobileMenu".
---
Nitpick comments:
In `@layouts/partials/header-new.html`:
- Line 202: The mobile overlay currently uses an <aside
class="mobile-menu-panel"> which is semantically for tangential content; change
this element to either a <div role="dialog"> or a native <dialog> to reflect its
modal behavior (preserve class="mobile-menu-panel" and all aria attributes like
aria-modal and aria-label/i18n). Update any JS/CSS that target the aside
selector (e.g., queries or styles referencing aside.mobile-menu-panel) to the
new selector (div.mobile-menu-panel or dialog.mobile-menu-panel) and ensure
focus management/aria attributes remain unchanged on the element used by the
mobile menu code.
- Around line 59-71: Create a new Hugo partial named menu-icon.html that accepts
the menu item name (use the passed-in .Name) and centralizes the mapping of
names to icon markup (Community→ri-team-line, Resource Modules→ri-apps-2-line,
Guidelines→ri-book-open-line, Blog Posts→ri-article-line, Config.
Management→ri-settings-3-line, Code of Conduct→ri-shield-check-line, Community
Calls→ri-calendar-event-line, FAQ→ri-question-answer-line,
Help→ri-lifebuoy-line, default→ri-file-text-line); then replace each duplicated
icon block in header-new.html (the desktop dropdown block using .Name, the
mobile submenu block, and the mobile top-level items) with a call to the
partial: pass .Name into partial "menu-icon.html" so all three places use the
single source of truth for icons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 522f6429-46e7-48e1-abab-134cf0691441
📒 Files selected for processing (3)
i18n/en.tomli18n/fr.tomllayouts/partials/header-new.html
✅ Files skipped from review due to trivial changes (2)
- i18n/en.toml
- i18n/fr.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates the mobile menu so all navigation options are visible and easy to select. The menu now opens in a clearer mobile-friendly layout, includes the full list of links, and closes when selecting a link, tapping outside the menu, or pressing Escape.
What was shown before:
(whilst pressing the menu, it overlayed totally incorrectly).
To what it is with this change:
Also fixes a page styling issue that could affect how the menu and other styles are displayed.
This change is