feat: Implement site header with responsive navigation, mobile menu functionality.#21
Conversation
|
👋 Thanks for opening this pull request, @Jozz77! Before your PR is reviewed, please ensure:
🔍 Our team will review your PR shortly. If you have questions, feel free to ask in the comments. 🚀 Keep up the great work! — OWASP BLT |
📊 Monthly LeaderboardHi @Jozz77! Here's how you rank for March 2026:
Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run |
owasp-blt
bot
added
unresolved-conversations: 0
needs-peer-review
|
👋 Hi @Jozz77! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a responsive mobile navigation: header DOM now includes a hamburger/close toggle and a vertical mobile menu. Client JS toggles menu open/close, swaps icons, updates aria-expanded, highlights active links in mobile menu, and auto-closes on resize. Tailwind safelist updated for new utilities. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@_includes/header.html`:
- Around line 27-29: Add explicit button semantics and expose its controlled
region: update the element with id "mobile-menu-button" to include
type="button", add aria-controls pointing to "mobile-menu", and add an
aria-expanded attribute; then modify the toggle logic in assets/js/main.js (the
code that opens/closes the mobile menu and references the mobile-menu-button
element) to keep aria-expanded in sync by setting it to "true" when opening and
"false" when closing via setAttribute on the mobile-menu-button element.
In `@assets/js/main.js`:
- Around line 89-131: The open/close timeouts for the mobile menu are not
cancelled, causing stale timers to run and leave mobileMenu, menuIcon, and
closeIcon out of sync; introduce a module-scoped timer variable (e.g.,
mobileMenuTimer) and before calling any setTimeout in the click handler or the
resize listener call clearTimeout(mobileMenuTimer) to cancel the previous
pending animation, then assign the new setTimeout id to mobileMenuTimer; also
ensure any early-return code paths still clear the timer so the panel and icons
always end in the intended state.
- Around line 99-100: The runtime-only class 'max-h-[500px]' is added via
mobileMenu.classList.add('max-h-[500px]') in assets/js/main.js but may be purged
by Tailwind; update your Tailwind configuration to either include
assets/js/main.js (or its directory) in the content array or add 'max-h-[500px]'
to the safelist in tailwind.config.* so the utility is emitted in production
CSS, then rebuild and verify the utility exists in the generated CSS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 49179f1a-85bd-4556-aa18-4e76255c2aaf
📒 Files selected for processing (2)
_includes/header.htmlassets/js/main.js
e-esakman
left a comment
e-esakman
left a comment
There was a problem hiding this comment.
please fix the issues flagged by coderabbitai.
…te change. Also fixed exposing the menu state to assistive tech.
|
Fixed clearing pending animation timers before scheduling another state change. Also fixed exposing the menu state to assistive tech. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/js/main.js`:
- Around line 90-148: Replace the implicit "hidden" check with an explicit
boolean state (e.g., isMobileMenuOpen) tracked alongside mobileMenuTimer inside
the mobileMenuButton click handler and the window.resize handler: initialize
isMobileMenuOpen = false; on open set isMobileMenuOpen = true, remove 'hidden'
then run the timeout to add transition classes; on close set isMobileMenuOpen =
false, start the timeout to add 'hidden' after the transition; always use
isMobileMenuOpen to decide open vs close (instead of
mobileMenu.classList.contains('hidden')), and ensure window.resize clears
mobileMenuTimer, sets isMobileMenuOpen = false and resets the classes and
aria-expanded consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4b46f630-000f-4daf-ad65-69cb64b08858
📒 Files selected for processing (3)
_includes/header.html_layouts/default.htmlassets/js/main.js
🚧 Files skipped from review as they are similar to previous changes (1)
- _includes/header.html
arnavkirti
left a comment
arnavkirti
left a comment
There was a problem hiding this comment.
implement coderabbit suggestions, otherwise LGTM.
owasp-blt
bot
added
has-peer-review
There was a problem hiding this comment.
♻️ Duplicate comments (1)
assets/js/main.js (1)
89-104:⚠️ Potential issue | 🔴 CriticalCritical:
openMenuandcloseMenufunctions are called but never defined.Lines 92, 94, and 101 call
openMenu()andcloseMenu(), but these functions do not exist in the file. This will throw aReferenceErrorat runtime, completely breaking mobile navigation.The variables
mobileMenuTimer,menuIcon, andcloseIcon(lines 85-87) are declared but unused, confirming the function bodies were omitted.🐛 Proposed fix: Add the missing function definitions
if (mobileMenuButton && mobileMenu) { + const openMenu = () => { + if (mobileMenuTimer) { + clearTimeout(mobileMenuTimer); + mobileMenuTimer = null; + } + + mobileMenu.classList.remove('hidden'); + mobileMenuButton.setAttribute('aria-expanded', 'true'); + + mobileMenuTimer = setTimeout(() => { + mobileMenu.classList.remove('max-h-0', 'opacity-0'); + mobileMenu.classList.add('max-h-[500px]', 'opacity-100'); + mobileMenuTimer = null; + }, 10); + + menuIcon.classList.add('hidden'); + closeIcon.classList.remove('hidden'); + }; + + const closeMenu = (hideImmediately = false) => { + if (mobileMenuTimer) { + clearTimeout(mobileMenuTimer); + mobileMenuTimer = null; + } + + mobileMenu.classList.remove('max-h-[500px]', 'opacity-100'); + mobileMenu.classList.add('max-h-0', 'opacity-0'); + mobileMenuButton.setAttribute('aria-expanded', 'false'); + menuIcon.classList.remove('hidden'); + closeIcon.classList.add('hidden'); + + if (hideImmediately) { + mobileMenu.classList.add('hidden'); + return; + } + + mobileMenuTimer = setTimeout(() => { + mobileMenu.classList.add('hidden'); + mobileMenuTimer = null; + }, 300); + }; + mobileMenuButton.addEventListener('click', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/main.js` around lines 89 - 104, The event handlers reference missing functions openMenu and closeMenu (and declared-but-unused mobileMenuTimer, menuIcon, closeIcon), causing runtime errors; implement an openMenu() that sets mobileMenuButton.setAttribute('aria-expanded','true'), adds the visible class to mobileMenu, swaps menuIcon/closeIcon visibility and manages mobileMenuTimer for auto-close if needed, and implement closeMenu(force = false) that clears mobileMenuTimer, sets aria-expanded to 'false', removes the visible class from mobileMenu, swaps icons back, and when called with force (from the resize handler) immediately closes without waiting; ensure these functions are referenced by the existing mobileMenuButton and window resize listeners so the click and resize handlers work correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@assets/js/main.js`:
- Around line 89-104: The event handlers reference missing functions openMenu
and closeMenu (and declared-but-unused mobileMenuTimer, menuIcon, closeIcon),
causing runtime errors; implement an openMenu() that sets
mobileMenuButton.setAttribute('aria-expanded','true'), adds the visible class to
mobileMenu, swaps menuIcon/closeIcon visibility and manages mobileMenuTimer for
auto-close if needed, and implement closeMenu(force = false) that clears
mobileMenuTimer, sets aria-expanded to 'false', removes the visible class from
mobileMenu, swaps icons back, and when called with force (from the resize
handler) immediately closes without waiting; ensure these functions are
referenced by the existing mobileMenuButton and window resize listeners so the
click and resize handlers work correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b2aa54c5-7bcb-4e42-a45d-057fcc59840a
📒 Files selected for processing (1)
assets/js/main.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
assets/js/main.js (1)
90-99:⚠️ Potential issue | 🟠 MajorClear pending close timer in
openMenu()to prevent race condition.If the user clicks the menu button during the 300ms close transition,
openMenu()is called but the pendingmobileMenuTimerfromcloseMenu()will still fire and addhidden, leaving the menu in a broken state.🛠️ Proposed fix
const openMenu = () => { + if (mobileMenuTimer) { + clearTimeout(mobileMenuTimer); + mobileMenuTimer = null; + } mobileMenu.classList.remove('hidden'); // Trigger reflow for transition mobileMenu.offsetHeight;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/main.js` around lines 90 - 99, openMenu currently doesn't cancel the pending close timer from closeMenu, so a previously set mobileMenuTimer can still fire and re-hide the menu; update openMenu to check for an active mobileMenuTimer, call clearTimeout(mobileMenuTimer) and set mobileMenuTimer = null (or undefined) before performing the open steps; ensure mobileMenuTimer is the same timer variable used by closeMenu and is in scope so the timeout is properly cancelled.
🧹 Nitpick comments (1)
assets/js/main.js (1)
89-89: Consider guarding against missing icon elements.The condition only checks for
mobileMenuButtonandmobileMenu, butmenuIconandcloseIconare accessed unconditionally inside. If the HTML structure changes or elements fail to load, this will throwTypeError: Cannot read properties of null.♻️ Suggested defensive check
- if (mobileMenuButton && mobileMenu) { + if (mobileMenuButton && mobileMenu && menuIcon && closeIcon) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/main.js` at line 89, The click/toggle block checks only mobileMenuButton and mobileMenu but later references menuIcon and closeIcon unconditionally; update the guard to also verify menuIcon and closeIcon are non-null (or early-return if any are missing) before using them, e.g., expand the if condition that includes mobileMenuButton and mobileMenu to include menuIcon and closeIcon or add explicit null checks inside the handler for the menuIcon and closeIcon elements referenced in the toggle logic (symbols: mobileMenuButton, mobileMenu, menuIcon, closeIcon).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@assets/js/main.js`:
- Around line 90-99: openMenu currently doesn't cancel the pending close timer
from closeMenu, so a previously set mobileMenuTimer can still fire and re-hide
the menu; update openMenu to check for an active mobileMenuTimer, call
clearTimeout(mobileMenuTimer) and set mobileMenuTimer = null (or undefined)
before performing the open steps; ensure mobileMenuTimer is the same timer
variable used by closeMenu and is in scope so the timeout is properly cancelled.
---
Nitpick comments:
In `@assets/js/main.js`:
- Line 89: The click/toggle block checks only mobileMenuButton and mobileMenu
but later references menuIcon and closeIcon unconditionally; update the guard to
also verify menuIcon and closeIcon are non-null (or early-return if any are
missing) before using them, e.g., expand the if condition that includes
mobileMenuButton and mobileMenu to include menuIcon and closeIcon or add
explicit null checks inside the handler for the menuIcon and closeIcon elements
referenced in the toggle logic (symbols: mobileMenuButton, mobileMenu, menuIcon,
closeIcon).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: cc9a4ceb-a62e-499b-abfe-716e30feb409
📒 Files selected for processing (1)
assets/js/main.js
|
@arnavkirti Please I've implemented the coderabbit suggestions. |
DonnieBLT
left a comment
DonnieBLT
left a comment
There was a problem hiding this comment.
please fix the conflicts
…Fix-Navigation-hamburger-menu-icon-missing-on-mobile-viewports
There was a problem hiding this comment.
🧹 Nitpick comments (1)
assets/js/main.js (1)
14-29: Trim filter text and null-guarddata-searchbefore.includes().This keeps whitespace-only input from acting like a real query and prevents a runtime error if a card is missing
data-search.♻️ Suggested hardening
- const searchValue = searchFilter.value.toLowerCase(); + const searchValue = searchFilter.value.trim().toLowerCase(); ... - const courseSearch = course.getAttribute('data-search'); + const courseSearch = course.getAttribute('data-search') ?? ''; ... - const searchMatch = !searchValue || courseSearch.includes(searchValue); + const searchMatch = !searchValue || courseSearch.includes(searchValue);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/main.js` around lines 14 - 29, The filter logic uses raw input and directly calls .includes on a possibly missing attribute; trim and null-guard the values: call .trim().toLowerCase() on searchFilter.value (and optionally categoryFilter.value and levelFilter.value) when creating searchValue/categoryValue/levelValue, and guard course.getAttribute('data-search') before using .includes — e.g., normalize courseSearch to an empty string if null, then perform the includes check in the course filtering block (references: searchFilter, categoryFilter, levelFilter, coursesGrid, 'course-card', data-search).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@assets/js/main.js`:
- Around line 14-29: The filter logic uses raw input and directly calls
.includes on a possibly missing attribute; trim and null-guard the values: call
.trim().toLowerCase() on searchFilter.value (and optionally categoryFilter.value
and levelFilter.value) when creating searchValue/categoryValue/levelValue, and
guard course.getAttribute('data-search') before using .includes — e.g.,
normalize courseSearch to an empty string if null, then perform the includes
check in the course filtering block (references: searchFilter, categoryFilter,
levelFilter, coursesGrid, 'course-card', data-search).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: cdfd9a43-d760-4150-bdb2-3ec4190f934b
📒 Files selected for processing (1)
assets/js/main.js
|
@DonnieBLT thanks for your feedback Merge conflicts have now been resolved |
…Fix-Navigation-hamburger-menu-icon-missing-on-mobile-viewports
4 participants
Implement Responsive Mobile Navigation
Clear description of changes
Motivation and context
Previously, the site's navigation was completely inaccessible on mobile devices. When the viewport width dropped below 768px, the horizontal menu would disappear without any alternative navigation method. These changes ensure a fully responsive experience, allowing mobile users to navigate between courses and other university pages easily.
Related issues
Fixes #20
Screenshots
Testing performed
Summary by CodeRabbit