-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat(old-theme): [dropdown,grid,tree] modify incorrect css vars #2551
Conversation
WalkthroughThe changes in this pull request involve updates to the CSS styles for the dropdown, grid, and tree components. Specifically, a new CSS property Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
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: 1
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/dropdown/index.less (1)
77-79
: Consider adding a comment explaining the min-width override.The button group styling is well-structured with clear variable usage. Consider adding a brief comment explaining why
min-width: auto
is necessary, as this would help future maintainers understand the intent behind this override..@{dropdown-prefix-cls}__title-button { padding: var(--tv-Dropdown-title-button-padding); border-right: none; border-radius: var(--tv-Dropdown-title-button-border-radius); + /* Allow button to shrink based on content */ min-width: auto; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/theme/src/dropdown/index.less
(1 hunks)packages/theme/src/grid/vars.less
(1 hunks)packages/theme/src/tree/vars.less
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/src/grid/vars.less
🔇 Additional comments (2)
packages/theme/src/dropdown/index.less (1)
78-78
: Verify the layout impact of removing minimum width constraint.
The addition of min-width: auto
will allow the title button to shrink below any default minimum width. While this can help prevent unwanted spacing, please verify that:
- The button content remains properly visible
- The layout works well with different content lengths
- The alignment with the caret button is maintained
✅ Verification successful
Based on the search results, I can see that:
-
The
min-width: auto
property is already used in several other components in the codebase, including:.dropdown-prefix-cls__title-button
(the current change).button-group-prefix-cls__item
in button-group.query-builder-prefix-cls__group-item
in query-builder.grid-toolbar-prefix-cls__item
in grid-toolbar.badge-prefix-cls__dot
in badge
-
The dropdown component has another button with
min-width: 24px
which suggests that the layout is already designed to handle different button widths. -
The button group component (which is related to this change) specifically uses
min-width: auto
for its items, indicating this is a consistent pattern for button groups in the design system.
Given this evidence, I can now provide the final response:
The min-width: auto
change is consistent with existing patterns
The use of min-width: auto
is a well-established pattern in the codebase, particularly in button groups and similar components. This change aligns with how other components handle flexible button widths while maintaining layout integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom min-width overrides or related button width constraints
# that might conflict with this change
# Check for min-width usage in button related styles
rg "min-width" --type less
# Look for button width related variables in theme files
rg "(button|btn).*(width|size)" --type less
Length of output: 28971
packages/theme/src/tree/vars.less (1)
Line range hint 1-108
: Well-structured CSS variables with comprehensive documentation
The file maintains excellent documentation standards with:
- Clear copyright notice
- Organized variable grouping
- Bilingual comments for each variable
- Consistent naming conventions
@@ -34,7 +34,7 @@ | |||
// 选中节点背景色 | |||
--tv-Tree-node-content-current-bg-color: var(--tv-color-bg-secondary, #ffffff); | |||
// 节点悬浮背景色 | |||
--tv-Tree-node-content-hover-bg-color: var(--tv-color-bg, #f5f5f5); | |||
--tv-Tree-node-content-hover-bg-color: var(--tv-color-bg-hover, #f5f5f5); |
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.
💡 Codebase verification
Inconsistent hover background color variables need standardization
Several components are still using --tv-color-bg
for hover states instead of the semantically correct --tv-color-bg-hover
:
packages/theme/src/transfer/vars.less
:--tv-Transfer-panel-item-hover-bg-color
packages/theme/src/pager/vars.less
:--tv-Pager-page-item-bg-color-hover
packages/theme/src/drop-times/vars.less
:--tv-DropTimes-item-select-hover-bg-color
packages/theme/src/cascader-node/vars.less
:--tv-CascaderNode-selectable-hover-bg-color
packages/theme/src/button/vars.less
:--tv-Button-bg-color-only-icon-ontext-hover
While this PR correctly updates the Tree component, similar standardization is needed across these other components to maintain consistency in hover state styling.
🔗 Analysis chain
Verify the hover state consistency across components
The change from --tv-color-bg
to --tv-color-bg-hover
aligns with similar updates in the grid component and provides a more semantically correct variable for hover states.
Let's verify this change is consistent across related components:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for hover background color variables across components
# to ensure consistent usage of --tv-color-bg-hover
# Search for hover background color variables
rg --type=less "hover-bg-color.*tv-color-bg" packages/theme/src/
# Search for any remaining usage of --tv-color-bg in hover contexts
rg --type=less "hover.*tv-color-bg[^-]" packages/theme/src/
Length of output: 3722
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes