-
Notifications
You must be signed in to change notification settings - Fork 276
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
fix(base-select): fix size style #2541
Conversation
WalkthroughThe changes involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (7)packages/theme/src/base-select/vars.less (2)
The new tag height variables follow a consistent naming pattern and calculation approach, subtracting 8px from their respective base heights. The implementation is clean and maintainable.
The calculations reference base size variables ( Also applies to: 67-67, 69-69, 71-71 ✅ Verification successfulBased on the search results, I can see that the base size height variables are properly defined in the theme files:
--tv-size-height-xs: 24px; // mini 尺寸
--tv-size-height-sm: 28px; // small 尺寸
--tv-size-height-md: 32px; // medium 尺寸 - 默认
--tv-size-height-lg: 40px; // large 尺寸
--tv-BaseSelect-tag-height-medium: calc(var(--tv-size-height-lg) - 8px); The referenced size variables are properly defined and available in the theme system. The variables are defined in the base theme file and are widely used across multiple components, confirming their availability and proper integration into the theming system. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for the definition of base size height variables
# Expected: Find definitions of the base size variables in theme files
# Search for size height variable definitions
rg --type=less "tv-size-height-(lg|md|sm|xs)\s*[,:]" packages/theme/
# Search for potential imports that might define these variables
rg --type=less "@import.*size|height" packages/theme/
Length of output: 111463 packages/theme/src/base-select/index.less (5)
The change from a fixed pixel value to a CSS variable improves maintainability and flexibility.
The change maintains consistency with the default size implementation.
The changes maintain consistent spacing and follow a logical size progression in the codebase.
The implementation looks good, but we should verify that all the new CSS variables are properly defined in
✅ Verification successfulAll required CSS variables are properly defined in vars.less The verification confirms that all necessary CSS variables for tag heights are correctly defined in
Each variable follows a consistent pattern of subtracting 8px from the corresponding base size height. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify CSS variable definitions in vars.less
# Check for the presence of all required CSS variables
rg --type less "tv-BaseSelect-tag-height-(default|medium|small|mini)" packages/theme/src/base-select/vars.less
Length of output: 410
While the height changes look good, please verify that these modifications don't affect the vertical alignment of tags, particularly when:
This will ensure the changes don't introduce any visual regressions. ✅ Verification successfulHeight changes are safe for vertical alignment The changes to tag heights are well-supported by the existing layout system:
The vertical alignment will be maintained across all states because:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Look for related vertical alignment styles
# Check for vertical alignment related properties
rg --type less "(vertical-align|align-items|display: flex)" packages/theme/src/base-select/
Length of output: 821 Script: #!/bin/bash
# Let's check the context around these alignment properties and tag-related styles
rg --type less -B 2 -A 2 "(vertical-align|align-items|display: flex)" packages/theme/src/base-select/
# Also check for any tag-specific styles that might affect alignment
rg --type less -B 2 -A 2 "@{tag-prefix-cls}" packages/theme/src/base-select/
Length of output: 3685 Script: #!/bin/bash
# Let's check tag-related styles with proper escaping
rg --type less -B 2 -A 2 "\@\{tag-prefix-cls\}" packages/theme/src/base-select/
# Also check for any styles related to tag positioning and spacing
rg --type less -B 2 -A 2 "(margin|padding|gap)" packages/theme/src/base-select/
Length of output: 6900 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 (
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
PR
主要更新:将写死的多选tag高度动态计算,以适配多种主题。
新老主题效果对比:
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