fix: navbar state change #4406
fix: navbar state change #4406
Conversation
🦋 Changeset detectedLatest commit: 893681d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request addresses a bug in the NextUI navbar component that prevented the mobile menu from opening correctly. The changes are focused on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@jrgarciadev @wingkwong This occurs because in order to test we would manually change the width of the device, so |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/smart-oranges-peel.md (1)
1-6: Enhance the changeset description with more detailsThe changeset description could be more informative by including:
- The root cause (usePreventScroll width calculation issue)
- The specific fix implemented (scroll width handling)
- Impact on existing behavior
Consider updating the description to:
--- "@nextui-org/navbar": patch --- -Resolving the issue preventing the navbar from opening(#4345) +Fix navbar menu closing unexpectedly due to usePreventScroll width calculations (#4345) + +- Prevents unnecessary width updates when only scroll width changes +- Maintains existing behavior for initial click functionality
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/smart-oranges-peel.md(1 hunks)packages/components/navbar/src/use-navbar.ts(1 hunks)
🔇 Additional comments (2)
packages/components/navbar/src/use-navbar.ts (2)
164-168: Note: Existing first-click behavior preserved as intended
As mentioned in the PR comments, the implementation intentionally maintains the existing behavior where the first click might require a width calculation. This is the correct approach as:
- It's not a regression but existing behavior
- The fix focuses solely on resolving the unintended width calculation issue
164-168: LGTM! Fix addresses the root cause effectively
The implementation correctly handles the width calculation issue by:
- Accounting for scroll width differences when usePreventScroll adds padding
- Preventing unnecessary navbar menu closures when only the scroll width changes
This fix directly addresses the issue described in #4345 where usePreventScroll was causing unexpected navbar closures.
Let's verify the scroll width calculation works across different scenarios:
✅ Verification successful
No other instances require similar scroll width fixes
The verification shows that:
- The scroll width calculation is unique to the navbar component and isn't used elsewhere in the codebase
- While usePreventScroll is used in other components (popover and aria-modal-overlay), they don't require width adjustments as they:
- Don't rely on resize observations
- Don't have width-dependent layout behaviors like the navbar
The current fix is correctly scoped to the navbar component where the issue manifests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other scroll width calculations in the codebase
# to ensure consistent handling
# Look for other scroll width calculations
rg -A 2 "innerWidth.*clientWidth|clientWidth.*innerWidth"
# Check for other usePreventScroll usage that might need similar fixes
ast-grep --pattern 'usePreventScroll({$$$})'
Length of output: 1111
|
@macci001 could you please add the before behavior video vs the new behavior video ? |
Closes #4345
📝 Description
The PR fixes the Navbar unable to open issue.
Root cause:
usePreventScrolland removedreact-rmeove-scroll.usePreventScrollinternally adds theoverflow-hiddenandpaddingRightof{scrollBarWidth}styles to prevent the scrolling behaviour. Due to this the width of the navbar base would be changed. Now, this causes the useResizeObserver to trigger which in-turn closes the Navbar menu.The pr adds the changes to useResizeObserver callback so that it does not close the Navbar Menu.
Current Behaviour
WhatsApp.Video.2024-12-19.at.19.45.50.1.mp4
New Behaviour
WhatsApp.Video.2024-12-19.at.19.45.50.mp4
💣 Is this a breaking change (Yes/No): No
Summary by CodeRabbit
Bug Fixes
Improvements