-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix(dropdown): respect closeOnSelect prop on DropdownItem #2598
fix(dropdown): respect closeOnSelect prop on DropdownItem #2598
Conversation
🦋 Changeset detectedLatest commit: c33b918 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 ↗︎
|
WalkthroughThe update enhances the dropdown menu's functionality by ensuring that when the Changes
Assessment against linked issues
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 Configuration File (
|
@gtoxlili is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
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/menu/src/menu.tsx (3 hunks)
Additional comments: 3
packages/components/menu/src/menu.tsx (3)
- 2-2: The import statement has been updated to include the
Key
type from React, which is utilized in the newly addedonAction
function. This change is necessary for the implementation and ensures that theKey
type is available for use within the file.- 21-21: The
onAction
function has been renamed toonTopAction
. This renaming is part of the changes to ensure that thecloseOnSelect
property functions as expected. It's important to verify that all references to the oldonAction
function within this file and potentially in other parts of the codebase have been updated to reflect this change.Verification successful
The search results indicate that references to
onAction
are still present throughout the project, not only within themenu
component but also in other components likelistbox
,dropdown
,breadcrumbs
, and their respective stories, tests, and documentation. This suggests that the renaming ofonAction
toonTopAction
was specific to themenu.tsx
file and does not imply that all occurrences ofonAction
across the project should have been renamed. The presence ofonAction
in other components and contexts appears to be intentional and unrelated to the change made inmenu.tsx
.Given this context, the review comment about verifying that all references to the old
onAction
function within this file and potentially in other parts of the codebase have been updated to reflect this change is partially correct. It is accurate in the sense that withinmenu.tsx
, the renaming toonTopAction
should be consistent. However, the comment may imply a broader scope of renaming across the project, which is not supported by the evidence from the search results. The references toonAction
in other components and documentation are valid and expected to remain unchanged.Therefore, the verification process concludes that the review comment is correct regarding the need to ensure consistency within the
menu.tsx
file but may be misleading if interpreted as applying to the entire codebase. The renaming toonTopAction
seems to be a specific change within themenu
component to address a particular issue, and the presence ofonAction
in other parts of the codebase does not contradict this change.* 30-34: The addition of the new `onAction` function is a key part of the fix for the `closeOnSelect` issue. This function checks the `closeOnSelect` property's value and conditionally triggers `onTopAction` if `closeOnSelect` is true. This implementation allows individual `DropdownItem`s to override the default closing behavior of the dropdown menu. It's crucial to ensure that this new logic does not introduce any side effects, especially in scenarios where `closeOnSelect` might be dynamically changed or in complex dropdown structures.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old `onAction` function name within the project. rg --type tsx 'onAction'Length of output: 52
Script:
#!/bin/bash # Search for any remaining references to the old `onAction` function name within the project, without specifying file type. rg 'onAction'Length of output: 7457
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.
- please add a changeset
- please provide before & after videos to demonstrate if possible
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 reminder, I've added the changeset. |
Really waiting for this fix in the release) |
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.
Just tested with the same code from the issue, the menu is still closed after clicking New file. Also please add tests to cover this case.
<Dropdown>
<DropdownTrigger>
<Button variant="bordered">Open Menu</Button>
</DropdownTrigger>
<DropdownMenu aria-label="Static Actions">
<DropdownItem key="new" closeOnSelect={false}>
New file
</DropdownItem>
<DropdownItem key="copy">Copy link</DropdownItem>
<DropdownItem key="edit">Edit file</DropdownItem>
<DropdownItem key="delete" className="text-danger" color="danger">
Delete file
</DropdownItem>
</DropdownMenu>
</Dropdown>
Currently, if a I have added the relevant templates. Please test the changes based on the provided templates. |
@gtoxlili apart from storybook, I was asking for tests to be added in |
Added |
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.
Tests are not passing. Please check.
The previous changes I proposed indeed had some issues. I have fixed them, uploaded the updated version, and conducted further testing. @wingkwong |
The main reason for the issue is that the component's previous design coupled the onAction and closeOnSelect behaviors.
In the previous design, the user-provided onAction was merged with the close event based on DropdownMenu's closeOnSelect using mergeProps. The merged onAction was then passed to the item. When the event was triggered, the item directly executed the onAction method passed from the menu. (This is also the reason why the closeOnSelect of the item component was ineffective.) My previous modification did not consider the possibility of the user passing their own onAction. So, I chose to directly skip executing the onAction passed from DropdownMenu if the item had closeOnSelect. (I assumed mergeProps was an override-like setting, but it turned out to execute all events with the same name in sequence.) Now, I have changed the close event to:
Since the item passes its key when triggering onAction, I try to find whether it has closeOnSelect based on the key. I think this logic should be correct now (although the logic is still messy, mainly because this coupled design is not a good design). |
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.
Hey @gtoxlili please fix the conflicts |
@gtoxlili sorry for the delay, please fix the conflicts |
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.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- packages/components/dropdown/tests/dropdown.test.tsx (1 hunks)
- packages/components/dropdown/src/use-dropdown.ts (1 hunks)
- packages/components/dropdown/stories/dropdown.stories.tsx (2 hunks)
Additional comments not posted (4)
packages/components/dropdown/src/use-dropdown.ts (1)
155-165
: Ensureprops?.children
is correctly typed.The
onAction
function usesprops?.children?.find
to locate an item by key. Ensure thatprops?.children
is correctly typed as an array of elements with akey
property to avoid runtime errors.packages/components/dropdown/stories/dropdown.stories.tsx (2)
156-189
: Great addition for demonstratingcloseOnSelect
functionality.The
ItemCloseOnSelectTemplate
effectively showcases the behavior of dropdown items and menus with thecloseOnSelect
property set tofalse
. This is a valuable addition for testing and demonstrating the feature.
778-784
: Ensure consistency in export naming.The export
ItemCloseOnSelect
aligns with the naming convention used in other exports. Ensure that this naming is consistent across the codebase to maintain clarity and predictability.Verification successful
Export Naming Consistency Verified
The export
ItemCloseOnSelect
follows a descriptive naming convention consistent with other exports in the codebase. No inconsistencies were found in the naming pattern.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency in export naming for `ItemCloseOnSelect`. # Test: Search for all exports in the stories file to ensure naming consistency. rg --type tsx --no-heading --line-number --color never 'export const'Length of output: 99
Script:
#!/bin/bash # Locate the dropdown.stories.tsx file and search for all exports to ensure naming consistency. fd --type f --extension tsx --exec rg --no-heading --line-number --color never 'export const' {}Length of output: 35030
packages/components/dropdown/__tests__/dropdown.test.tsx (1)
799-833
: Excellent test coverage forcloseOnSelect
functionality.The test case "should respect closeOnSelect setting of DropdownItem" effectively verifies the new behavior. It ensures that the dropdown remains open when
closeOnSelect
is set tofalse
and closes otherwise. This is a critical addition to the test suite.
Closes #2290
📝 Description
This PR fixes an issue where the
closeOnSelect
prop onDropdownItem
was being overridden by theDropdownMenu
settings, causing the dropdown to close even whencloseOnSelect
was set tofalse
on an individualDropdownItem
.⛳️ Current behavior (updates)
Currently, setting
closeOnSelect
tofalse
on aDropdownItem
does not prevent the dropdown from closing when that item is selected. This is because theonAction
event handler onDropdownItem
is deprecated and the closing behavior is controlled by theDropdownMenu
settings.🚀 New behavior
The logic has been updated to respect the
closeOnSelect
prop onDropdownItem
. Now, ifcloseOnSelect
is set tofalse
on aDropdownItem
, selecting that item will not close the dropdown menu.The relevant changes are:
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
N/A
Summary by CodeRabbit
closeOnSelect
property for dropdown items, allowing them to remain open or close based on user selection.closeOnSelect
property.closeOnSelect
prop in the dropdown component.closeOnSelect
property in the DropdownItem component.