fix(dropdown): respect closeOnSelect prop on DropdownItem(#2290) #2598
Conversation
🦋 Changeset detectedLatest commit: 4b39f0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 to 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 Configration 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
Keytype from React, which is utilized in the newly addedonActionfunction. This change is necessary for the implementation and ensures that theKeytype is available for use within the file.- 21-21: The
onActionfunction has been renamed toonTopAction. This renaming is part of the changes to ensure that thecloseOnSelectproperty functions as expected. It's important to verify that all references to the oldonActionfunction 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
onActionare still present throughout the project, not only within themenucomponent but also in other components likelistbox,dropdown,breadcrumbs, and their respective stories, tests, and documentation. This suggests that the renaming ofonActiontoonTopActionwas specific to themenu.tsxfile and does not imply that all occurrences ofonActionacross the project should have been renamed. The presence ofonActionin 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
onActionfunction 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 toonTopActionshould 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 toonActionin 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.tsxfile but may be misleading if interpreted as applying to the entire codebase. The renaming toonTopActionseems to be a specific change within themenucomponent to address a particular issue, and the presence ofonActionin 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
Thanks for the reminder, I've added the changeset. |
|
Really waiting for this fix in the release) |
There was a problem hiding this comment.
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 |
It passes on my local machine, can you show me your test cases? |
|
@gtoxlili just look at the QA / Tests (pull_request). Already tried rerunning few times. I think your branch is out of sync. Please sync once with
|
I have already merged the canary branch into my fix branch and re-executed all the tests for the dropdown. These are the results on my local machine, and I still haven't found any errors. I don't have any clue how to solve this problem. If possible, perhaps you could provide me with some more detailed information?
|
|
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). |
Closes #2290
📝 Description
This PR fixes an issue where the
closeOnSelectprop onDropdownItemwas being overridden by theDropdownMenusettings, causing the dropdown to close even whencloseOnSelectwas set tofalseon an individualDropdownItem.⛳️ Current behavior (updates)
Currently, setting
closeOnSelecttofalseon aDropdownItemdoes not prevent the dropdown from closing when that item is selected. This is because theonActionevent handler onDropdownItemis deprecated and the closing behavior is controlled by theDropdownMenusettings.🚀 New behavior
The logic has been updated to respect the
closeOnSelectprop onDropdownItem. Now, ifcloseOnSelectis set tofalseon 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
menucomponent.