fix: should not export list item internal variables type#4200
fix: should not export list item internal variables type#4200winchesHe wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 3e8f7bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 type definitions related to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🔇 Additional comments (4)apps/docs/components/docs/components/code-demo/react-live-demo.tsx (2)
The change from
The type assertion to ✅ Verification successfulType assertion is safe in this context The type assertion is safe because:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check SandpackFiles type definition
rg -A 5 "export (type|interface) SandpackFiles"
Length of output: 49 Script: #!/bin/bash
# Try searching for SandpackFiles in node_modules
fd -g "*sandpack*.d.ts" -x cat {} \; | rg -A 5 "export (type|interface) SandpackFiles"
# Search for imports of SandpackFiles
rg "import.*SandpackFiles.*from"
# Search for the actual usage context
rg -B 5 -A 5 "value=\{files\?\.\[DEFAULT_FILE\] as string\}"
Length of output: 1720 Script: #!/bin/bash
# Let's check the actual usage of files in the component and its type
rg -B 10 -A 10 "const.*files.*:" apps/docs/components/docs/components/code-demo/react-live-demo.tsx
# Also check if there are any type definitions in the local codebase
ast-grep --pattern 'type SandpackFiles = $_'
Length of output: 148 Script: #!/bin/bash
# Let's examine the entire react-live-demo.tsx file to understand the context better
cat apps/docs/components/docs/components/code-demo/react-live-demo.tsx
# Also check how files are being used in other places
rg "files\?\.\[" --type typescript
Length of output: 2694 packages/components/listbox/src/use-listbox-item.ts (1)
The use of Also applies to: 32-35 ✅ Verification successfulLGTM: Consistent type usage between menu and listbox items The usage of
This ensures type consistency and proper variant handling across both components. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Verify MenuItemVariantProps usage consistency
ast-grep --pattern 'import { $$$, MenuItemVariantProps } from "@nextui-org/theme"'
Length of output: 321 Script: #!/bin/bash
# Check how MenuItemVariantProps is used in both files
ast-grep --pattern 'MenuItemVariantProps'
Length of output: 525 Script: #!/bin/bash
# Check how the variant props are used in the implementation
ast-grep --pattern 'mapPropsVariants<MenuItemVariantProps>'
Length of output: 61 Script: #!/bin/bash
# Check the implementation details in both files
rg "mapPropsVariants" -A 3 packages/components/listbox/src/use-listbox-item.ts packages/components/menu/src/use-menu-item.ts
Length of output: 1358 packages/components/menu/src/use-menu-item.ts (1)
The implementation mirrors the changes in Also applies to: 32-35 ✅ Verification successfulLet me verify the consistency of variant props handling between menu and listbox components more thoroughly. Verified: Consistent type handling between menu and listbox components The implementation shows perfect alignment between menu and listbox components:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Verify consistent usage of variant keys
rg -A 2 "menuItem.variantKeys|listboxItem.variantKeys"
Length of output: 488 Script: #!/bin/bash
# Check the implementation of mapPropsVariants in both files
ast-grep --pattern 'const $_ = mapPropsVariants($_,$_)'
# Check the type imports and their usage
rg -A 2 "MenuItemVariantProps"
Length of output: 7937 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: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/theme/src/components/menu.ts (1)
532-535: LGTM! Correctly prevents exposure of internal variant propertiesThe change appropriately excludes internal properties from the exported
MenuItemVariantPropstype while maintaining their internal usage for CSS class application.Consider adding a JSDoc comment explaining why these properties are excluded from the exported type:
+/** + * MenuItemVariantProps excludes internal properties used for CSS class application + * to prevent their exposure in the public API. + */ export type MenuItemVariantProps = Omit< VariantProps<typeof menuItem>, "hasDescriptionTextChild" | "hasTitleTextChild" >;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/components/menu/src/menu.tsx(1 hunks)packages/core/theme/src/components/menu.ts(1 hunks)
🔇 Additional comments (2)
packages/components/menu/src/menu.tsx (1)
77-77: Verify the impact of exposing internal properties
The removal of Omit utility exposes internal properties (hasChildItems, hasTitleTextChild, hasDescriptionTextChild) through the MenuProps type. This could lead to unintended usage of these internal properties by consumers.
Let's check for any existing usage of these properties:
packages/core/theme/src/components/menu.ts (1)
Line range hint 150-161: LGTM! Well-structured internal variant handling
The implementation correctly handles text truncation as an internal UI concern. The variant properties are well-typed and appropriately scoped.
|
Hey @winchesHe I see no impactful changes here other than moving the types from one file to another, does this solve the mentioned issue? |
Yes, the original changes omit the internal types in the menu props, but it doesn't have, so i change it to the menuItem |
2 participants
Closes #
📝 Description
related #4157
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
New Features
MenuPropsandMenuItemVariantProps, enhancing flexibility in component usage.filesproperty in theReactLiveDemocomponent.Bug Fixes