Skip to content

fix: improve mobile hamburger menu behavior#794

Open
Abhijeet2409 wants to merge 1 commit into
OWASP:mainfrom
Abhijeet2409:fix-mobile-hamburger
Open

fix: improve mobile hamburger menu behavior#794
Abhijeet2409 wants to merge 1 commit into
OWASP:mainfrom
Abhijeet2409:fix-mobile-hamburger

Conversation

@Abhijeet2409

@Abhijeet2409 Abhijeet2409 commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR improves the mobile hamburger menu behavior and usability. The issue was previously addressed in PR #654, but that PR was closed due to inactivity. This PR resolves the issue with a simplified implementation.

Changes

  • Fixed the mobile menu height so it no longer occupies unnecessary space.
  • Added a close (X) button to the mobile hamburger menu for better usability.

Testing

Before Fix

Before Fix

After Fix

After Fix

Closes #646

@Abhijeet2409 Abhijeet2409 marked this pull request as ready for review March 6, 2026 19:14
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 5a2c3f04-b97c-412a-b0c0-fbe24f2472ba

📥 Commits

Reviewing files that changed from the base of the PR and between c0e28dd and 49f9bf6.

📒 Files selected for processing (2)
  • application/frontend/src/scaffolding/Header/Header.tsx
  • application/frontend/src/scaffolding/Header/header.scss
🚧 Files skipped from review as they are similar to previous changes (2)
  • application/frontend/src/scaffolding/Header/Header.tsx
  • application/frontend/src/scaffolding/Header/header.scss

Summary by CodeRabbit

  • New Features

    • Added a dedicated mobile menu close button with a clear “X” icon and an accessible “Close menu” label, positioned at the top-right for faster dismissal.
  • Bug Fixes

    • Improved the mobile menu container sizing so it adapts to the actual content height instead of consistently filling the entire screen.

Walkthrough

Adds a close button to the mobile navigation menu. The X icon is imported from lucide-react, a button element wired to closeMobileMenu is inserted into the mobile menu container, and a new .mobile-menu-close CSS rule positions it absolutely in the top-right corner. The .navbar__mobile-menu height is changed from 100% to fit-content.

Changes

Mobile Menu Close Button

Layer / File(s) Summary
Close button markup and styles
application/frontend/src/scaffolding/Header/Header.tsx, application/frontend/src/scaffolding/Header/header.scss
X icon added to lucide-react import; a mobile-menu-close button calling closeMobileMenu is inserted into the mobile menu JSX with a screen-reader-only "Close menu" label; .mobile-menu-close is styled as absolute-positioned in the top-right corner with white text color and high z-index; .navbar__mobile-menu height changed from 100% to fit-content.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: improve mobile hamburger menu behavior' directly summarizes the main change, addressing the core improvement to mobile menu usability.
Description check ✅ Passed The description clearly outlines the changes made (fixed menu height and added close button), references the resolved issue (#646), and includes before/after testing screenshots.
Linked Issues check ✅ Passed The PR directly addresses all coding requirements from issue #646: adding a close button to the mobile menu and fixing the menu height to prevent full-screen occupation.
Out of Scope Changes check ✅ Passed All changes (X icon import, close button implementation, menu height adjustment, and button styling) are directly scoped to fixing mobile menu behavior as required by issue #646.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@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: 3

🤖 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 `@application/frontend/src/scaffolding/Header/header.scss`:
- Around line 176-185: In the `.mobile-menu-close` selector in the header.scss
file, replace the hardcoded `color: white;` property with a CSS variable that
matches the theme pattern used throughout the stylesheet, such as `color:
var(--foreground);` or `color: var(--muted-foreground);`. This ensures the close
button text color adapts to the current theme and maintains proper contrast
across different theme variants, rather than always being white regardless of
the theme.
- Line 192: The header menu uses `height: fit-content` which prevents
unnecessary space occupation, but combined with `position: fixed` and no maximum
height constraint, the menu content can extend beyond the viewport on small
screens or with additional items, making content inaccessible. Add a
`max-height` property (set to a viewport-relative value such as `100vh` or
`calc(100vh - offsetValue)` to account for other fixed elements) and an
`overflow-y` property set to `auto` to the same selector containing `height:
fit-content`. This ensures the menu respects viewport boundaries and becomes
scrollable when content exceeds the available space.

In `@application/frontend/src/scaffolding/Header/Header.tsx`:
- Around line 138-140: The close button with className="mobile-menu-close" is
missing an accessible label for screen readers. Following the same accessibility
pattern used for the hamburger menu toggle which includes a span with
className="sr-only" containing "Toggle menu", add a similar span element inside
the close button that contains descriptive text like "Close menu" to inform
screen reader users of the button's purpose.
🪄 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: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: f29ab5e6-593e-48e9-97b3-5f42f665a632

📥 Commits

Reviewing files that changed from the base of the PR and between e853cd3 and f38bd2d.

📒 Files selected for processing (2)
  • application/frontend/src/scaffolding/Header/Header.tsx
  • application/frontend/src/scaffolding/Header/header.scss

Comment thread application/frontend/src/scaffolding/Header/header.scss
Comment thread application/frontend/src/scaffolding/Header/header.scss
Comment thread application/frontend/src/scaffolding/Header/Header.tsx
Signed-off-by: Abhijeet Saharan <abhijeetsaharan2236@gmail.com>
@Abhijeet2409 Abhijeet2409 force-pushed the fix-mobile-hamburger branch from c0e28dd to 49f9bf6 Compare June 14, 2026 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mobile Navbar Lacks Close Functionality In Full-Screen

1 participant