fix(listbox): change listBoxItem key to optional #3883
Conversation
🦋 Changeset detectedLatest commit: d89915d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
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
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/components/listbox/src/base/listbox-item-base.tsx (1 hunks)
- packages/components/select/tests/select.test.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/components/listbox/src/base/listbox-item-base.tsx (1)
94-94: Approve: Improved key handling for ListboxItemBasePropsThis change aligns well with the PR objectives by making the
keyproperty optional inListboxItemBaseProps. By omittingkeyfromAriaOptionProps, the component can now use theidfrom the item object directly, addressing the issue with numeric keys being converted to strings.Benefits of this change:
- Resolves the problem with numeric keys in the
Selectcomponent (issue [BUG] - Select with numeric keys are not supported #3880).- Maintains backwards compatibility while providing more flexibility.
- Aligns better with React's recommendations for key handling in lists.
To ensure this change doesn't introduce unintended side effects, please run the following verification:
This script will help identify any existing usage of the
keyprop inListboxItemcomponents, which might need attention due to this type change.✅ Verification successful
Verified: No issues found with omitting the
keyprop inListboxItemBasePropsThe change to omit the
keyproperty inAriaOptionPropshas been verified and does not introduce any issues. All existing usages of thekeyprop are confined to non-production files such as stories and documentation, ensuring that the component's core functionality remains unaffected.
- No
keyprop usages found in production component files.- Existing
keyusages in stories and docs are appropriate and non-impactful.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any direct usage of the 'key' prop in ListboxItem components # Test: Search for ListboxItem components with a 'key' prop rg --type typescript --type typescriptreact 'ListboxItem.*key=' -g '!*test*'Length of output: 113
Script:
#!/bin/bash # Description: Check for any direct usage of the 'key' prop in ListboxItem components by searching .ts and .tsx files # Search for ListboxItem components with a 'key' prop in .ts and .tsx files, excluding test files rg 'ListboxItem\s+[^>]*\bkey\s*=' --glob '!*.test.tsx' --glob '!*.test.ts'Length of output: 6469
packages/components/select/__tests__/select.test.tsx (1)
353-376: Test cases effectively cover numeric key handlingThe added test cases correctly verify the handling of numeric keys and IDs in the
Selectcomponent. They ensure that items are pre-selected as expected and thatonSelectionChangecallbacks receive the correct values. The assertions are appropriate, and the tests enhance the robustness of the component.Also applies to: 378-401, 405-422, 445-462
ryo-manba
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.changeset/curly-zoos-thank.md (1)
1-5: LGTM! Changeset looks good.The changeset correctly identifies this as a patch update for the "@nextui-org/listbox" package, which is appropriate for a backwards-compatible bug fix. The change description is concise and references the relevant issue.
Consider expanding the change description slightly for more clarity:
-change ListboxItem key to optional (#3880) +change ListboxItem key to optional to support numeric keys (#3880)This addition provides more context about the purpose of the change without significantly increasing the length of the description.
Closes #3880
📝 Description
In
SelectBoxand other collections, whenitemsincludeidorkey, these values are treated as the component'skey.This allows numeric values to remain as numbers. However, using
keydirectly in the component (e.g,<SelectItem key={1} />) results in the value being converted to a string.This PR makes
keyoptional sinceidfrom the item object can be used directly.Reference: ListBox – React Aria
⛳️ Current behavior (updates)
keyis required and always converted to a string.🚀 New behavior
keyis now optional.💣 Is this a breaking change (Yes/No):
No.
📝 Additional Information
Summary by CodeRabbit
New Features
Selectcomponent with improved pre-selection functionality based on numeric keys and IDs.Bug Fixes
Selectcomponent.Tests
Selectcomponent to improve functionality and coverage.