refactor: remove unnecessary className passing to tv and make naming consistent #4558
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces a series of modifications across multiple component hooks in the packages/components directory. The primary change involves removing Changes
Suggested Labels
Suggested Reviewers
Possibly Related PRs
Finishing Touches
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
🔭 Outside diff range comments (1)
packages/components/dropdown/src/use-dropdown.ts (1)
Line range hint
127-132: Keep naming consistent with other components.The variable name
stylesintroduces inconsistency with other components that useclassNames. Consider keeping the naming convention consistent across the codebase.- const styles = useMemo( + const slots = useMemo( () => dropdown({ className, }), [className], );
🧹 Nitpick comments (1)
packages/components/select/src/use-select.ts (1)
Line range hint
248-248: Overall changes look good and align with PR objectivesThe changes consistently remove
classNamefrom the dependency arrays ofuseMemohooks across all components. This is a valid optimization as:
classNameis only used for base styling- It doesn't affect the slot variants computation
- Reduces unnecessary recalculations of the slots object
The changes improve performance while maintaining the correct functionality.
Also applies to: 282-282, 389-389, 385-385
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/components/autocomplete/src/use-autocomplete.ts(1 hunks)packages/components/breadcrumbs/src/use-breadcrumbs.ts(1 hunks)packages/components/button/src/button.tsx(1 hunks)packages/components/button/src/use-button.ts(2 hunks)packages/components/code/src/use-code.ts(2 hunks)packages/components/date-input/src/use-date-input.ts(1 hunks)packages/components/date-input/src/use-time-input.ts(1 hunks)packages/components/date-picker/src/use-date-range-picker.ts(1 hunks)packages/components/dropdown/src/use-dropdown.ts(2 hunks)packages/components/link/src/use-link.ts(2 hunks)packages/components/listbox/src/use-listbox.ts(1 hunks)packages/components/select/src/use-select.ts(1 hunks)packages/components/slider/src/use-slider.ts(1 hunks)packages/components/tabs/src/use-tabs.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/components/code/src/use-code.ts
- packages/components/link/src/use-link.ts
🔇 Additional comments (11)
packages/components/button/src/button.tsx (1)
27-27: LGTM! Clean className handling.The change correctly removes redundant className passing and relies on getButtonProps for style management.
packages/components/listbox/src/use-listbox.ts (1)
134-134: Verify empty dependency array impact.While removing className from tv call is correct, ensure that the empty dependency array won't prevent necessary slot updates if variant props change.
✅ Verification successful
Empty dependency array is correct and safe
The listbox component's slots are static and don't depend on any variant props. All variant-dependent styles are handled by the listboxItem component separately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if listbox has variant-dependent styles ast-grep --pattern 'export const listbox = tv({ $$$ variants: { $$$ } $$$ })'Length of output: 91
Script:
#!/bin/bash # Find files that might contain listbox definition fd -t f listbox # Search for listbox definition with tv rg "const listbox.*=.*tv\(" -A 10Length of output: 1049
Script:
#!/bin/bash # Check theme file content cat packages/core/theme/src/components/listbox.ts # Broader search for tv with variants rg "tv\({[^}]*variants:" -A 20 packages/core/theme/src/components/listbox.tsLength of output: 1970
Script:
#!/bin/bash # Check menu component implementation cat packages/core/theme/src/components/menu.ts # Search for tv definitions in menu rg "export const menu = tv" -A 20 packages/core/theme/src/components/menu.tsLength of output: 15101
packages/components/breadcrumbs/src/use-breadcrumbs.ts (1)
154-154: LGTM! Proper dependency handling.The change correctly uses objectToDeps for tracking variant prop changes while removing unnecessary className dependency.
packages/components/tabs/src/use-tabs.ts (1)
130-130: Optimization: Removed unnecessary className dependencyThe removal of
classNamefrom the dependency array is correct as the slots don't need to be recalculated when only the className changes. The className is properly merged in the prop getters usingclsx.packages/components/button/src/use-button.ts (1)
179-179: LGTM: Consistent style handlingThe addition of
stylesto both the props and dependency array ensures proper style application and updates. This change aligns with the PR's goal of making naming more consistent.Also applies to: 193-193
packages/components/date-input/src/use-time-input.ts (1)
156-156: LGTM: Optimized slots memoizationRemoving
classNamefrom the dependency array is correct as the slots don't need to be recalculated when only the className changes. The className is still properly handled in the prop getters.packages/components/date-input/src/use-date-input.ts (1)
214-214: LGTM: Consistent with time input changesThe removal of
classNamefrom the dependency array matches the changes in the time input component, maintaining consistency across date-related components while optimizing the slots memoization.packages/components/slider/src/use-slider.ts (1)
248-248: Optimization approved: Removed unnecessary className dependencyThe removal of
classNamefrom the dependency array is correct as it's only used for base styling and doesn't affect the slot variants computation.packages/components/date-picker/src/use-date-range-picker.ts (1)
282-282: Optimization approved: Removed unnecessary className dependencyThe removal of
classNamefrom the dependency array is correct as it's only used for styling and doesn't affect the slot variants computation.packages/components/autocomplete/src/use-autocomplete.ts (1)
389-389: Optimization approved: Removed unnecessary className dependencyThe removal of
classNamefrom the dependency array is correct as it's only used for base styling and doesn't affect the slot variants computation.packages/components/select/src/use-select.ts (1)
385-385: Optimization approved: Removed unnecessary className dependencyThe removal of
classNamefrom the dependency array is correct as it's only used for base styling and doesn't affect the slot variants computation.
Closes #
📝 Description
When you pass
classNametotv, it is passing to base slot. Currently there are 3 types of patterns in our codebase.This PR does two things
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Performance Optimization
classNamefrom various component memoization hooks to reduce unnecessary recalculationsStyling Updates
classNamestostylesin multiple componentsDeprecation Notice
disableClearableprop for removal in a future minor releaseThe changes appear to be part of a systematic refactoring to optimize component performance and standardize styling approaches across the component library.