New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat direct control navbar-menu #2547
base: canary
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@valguzin55 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe update introduces a new Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- packages/components/navbar/src/navbar-menu.tsx (2 hunks)
Additional comments: 4
packages/components/navbar/src/navbar-menu.tsx (4)
- 24-27: The introduction of the
isOpen
prop is a significant enhancement, allowing for direct control over the navbar menu's open/close state. This change aligns with the PR's objective to provide developers with more flexibility in managing menu visibility, especially in scenarios involving multipleNavbarMenu
components.- 31-49: The logic for determining the
isMenuOpen
state based on theisOpen
prop is correctly implemented. However, it's essential to ensure that this change does not inadvertently affect existing implementations that rely on the internal state management of theNavbarMenu
component. Consider adding documentation or migration guidelines if this change could impact existing users.- 54-61: The use of
useCallback
for theMenuWrapper
component is appropriate, given that it depends on theisMenuOpen
state. This optimization helps avoid unnecessary re-renders of theMenuWrapper
component when theisMenuOpen
state changes.- 21-71: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [68-88]
The handling of the
data-open
attribute and the conditional rendering based on theisMenuOpen
state are well-implemented. These changes ensure that the menu's visibility is correctly managed according to theisOpen
prop or the internal state from theNavbarContext
. However, it's crucial to test these changes thoroughly, especially in scenarios where theisOpen
prop might change frequently or where multipleNavbarMenu
components are used within a single navbar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Please provide the following items:
- changeset
- a test case for handling this behaviour
- an example in docs
Also please lint the code. some changes like className={slots.menu?.({ class: styles })}
shouldn't be included.
Closes #
📝 Description
add direct control state navbar-menu open/close for 2 or more navbarmenu in navbar
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
isOpen
property.