fix: adjust navbar breakpoints to lg to prevent overflow #3272#3324
fix: adjust navbar breakpoints to lg to prevent overflow #3272#3324kasya merged 6 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe Header component's responsive breakpoints are adjusted, shifting desktop header links visibility from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 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: 1
🤖 Fix all issues with AI agents
In @frontend/src/components/Header.tsx:
- Line 61: The breakpoint constant desktopViewMinWidth is still 768 causing JS
to mismatch the CSS lg breakpoint; update desktopViewMinWidth from 768 to 1024
in frontend/src/utils/constants.ts, ensure the useIsMobile hook (which reads
desktopViewMinWidth) will now treat widths <1024 as mobile, and adjust the
Header.tsx handleResize logic (and any checks referencing desktopViewMinWidth or
directly using 768 in Header.tsx) so the mobile menu closes at >=1024; also
update the unit test mock in frontend/__tests__/unit/pages/Header.test.tsx to
use 1024 instead of 768 so tests reflect the new breakpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/Header.test.tsx (1)
107-130: LGTM! Mock value correctly synchronized with the updated constant.The mock value
desktopViewMinWidth: 1024matches the actual constant inconstants.ts.Consider importing the actual constant instead of hardcoding the value in the mock. This would prevent the values from drifting out of sync in future updates:
♻️ Suggested improvement
+import { desktopViewMinWidth, headerLinks } from 'utils/constants' + // Mock constants -jest.mock('utils/constants', () => ({ - desktopViewMinWidth: 1024, - headerLinks: [ +jest.mock('utils/constants', () => { + const actual = jest.requireActual('utils/constants') + return { + ...actual, // Override only if needed for specific test scenarios - ... - ], -})) + } +})Alternatively, if the full mock is intentional for test isolation, the current approach is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/pages/Header.test.tsxfrontend/src/utils/constants.ts
🔇 Additional comments (2)
frontend/__tests__/unit/pages/Header.test.tsx (1)
191-207: Consistent test setup with the new breakpoint.The
globalThis.innerWidthvalue of 1024 on line 198 correctly aligns with the updateddesktopViewMinWidthconstant, ensuring tests run at the desktop view threshold.frontend/src/utils/constants.ts (1)
125-125: Breakpoint constant correctly aligned with Tailwind'slgbreakpoint.The change to 1024 consistently applies across the codebase:
- JS constant matches Tailwind's
lgbreakpoint- CSS classes properly updated (
lg:block,lg:table-cell, etc.)- Test mocks synchronized to 1024
- All usages (Header resize handler, useIsMobile hook) operate correctly with the new value
|
Thank you for the suggestion! I've synchronized the hardcoded values to 1024 across the constants and tests to ensure consistency, but I’m choosing to keep the code simple and consistent with how the rest of the project is written. |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@harshita8120 hi! Thanks for working on this! Left a question/request.
Also, please resolve CodeRabbit comment.
Also please run make check-test and address found issues before pushing to remote - you haven't done that this time.
There was a problem hiding this comment.
This works good overall 👍🏼
One thing I noticed is that when we are at a 1024 and collaps some of the menus - the buttons are now still showing in the Header until 768 breakpoint. However, we also have them in the menu on the side. This causes Star and Sponsor buttons to show up in both places.
Do you think you can update that?
There was a problem hiding this comment.
Yeah, sure, I'll shortly update this.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/pages/Header.test.tsx`:
- Around line 106-126: Remove the duplicate "// Mock constants" comment and
replace the vague "// Matches your logic" with a clearer note describing the
breakpoint; update the jest.mock block that overrides utils/constants (the
desktopViewMinWidth and headerLinks mock) to have a single "// Mock constants"
header and change the desktopViewMinWidth inline comment to something like "//
Updated breakpoint threshold for lg screens" so the purpose is explicit.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Header.tsx (1)
27-32: UpdatedesktopViewMinWidthfrom 768 to 1024 infrontend/src/utils/constants.ts.The resize handler closes the mobile menu when
innerWidth >= desktopViewMinWidth, but the constant is currently set to 768. This creates a mismatch: the mobile menu closes at 768px while the menu toggle button remains visible until 1024px (thelgCSS breakpoint). Update the constant to 1024 to ensure consistency across all breakpoint logic.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@harshita8120 thanks for working on this!
I pushed a little change to update how we show buttons: we'll show them only on the top header until 768px width, then move to the sidebar 👌🏼
2 participants
Resolves #3272 - Fix navbar items overflow on medium screens
This PR addresses the issue where navbar items would overlap or become inaccessible on medium-sized screens (768px - 1024px). Earlier in the specified screen size range (768px - 1024px), the sign in and theme change buttons were inaccessible due to overflow of the items in the navbar.
Changes Made:
Updated the responsive breakpoints in "Header.tsx" from md to lg.
Screenshots for different screen sizes after the changes were made:
Checklist
make check-testlocally: all warnings addressed, tests passed