fix(button): [button]set svg size when button is only icon #2380
Conversation
WalkthroughThe changes in this pull request enhance the button component's styling capabilities by introducing new variables for icon sizes in the 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 (
|
![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 (2)
packages/theme/src/button/index.less (1)
44-54: LGTM! Consider enhancing the documentation.The implementation correctly sets SVG icon sizes for icon-only buttons using CSS custom properties. The approach is consistent with the existing codebase patterns and effectively solves the sizing issue.
Consider adding a brief English translation alongside the Chinese comment for better international developer experience:
- // 仅图标时，图标的大小 + // 仅图标时，图标的大小 + // Set icon size when button contains only an iconpackages/theme/src/button/vars.less (1)
38-39: Consider documenting the icon size selection rationale.While the sizes are appropriate, it would be helpful to document:
- The reasoning behind the size progression
- Any design system guidelines these sizes align with
- Recommended usage contexts for each size
Add a comment block above the first icon size variable explaining these aspects:
+ // Icon size guidelines: + // - Sizes follow a 12/14/18/24/28px progression + // - Default (18px): Standard buttons and most common use case + // - Large (28px): Hero sections, featured actions + // - Medium (24px): Secondary featured actions + // - Small (14px): Compact UI areas + // - Mini (12px): Dense UI areas, toolbar actions // 默认时按钮内图标的大小 --tv-Button-size-icon-font-size: 18px;Also applies to: 48-49, 58-59, 68-69, 78-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/button/index.less (1 hunks)
- packages/theme/src/button/vars.less (5 hunks)
🔇 Additional comments (4)
packages/theme/src/button/index.less (1)
44-54: Verify CSS custom properties and design system alignment.While the implementation is correct, please ensure:
- All required CSS custom properties (
--tv-Button-size-icon-font-size*) are properly defined invars.less.- The icon sizes align with the design system specifications for each button size variant.
Let's verify the CSS custom properties:
✅ Verification successful
Since the shell script results show that all the necessary CSS custom properties for button icon sizes (
--tv-Button-size-icon-font-size*) are properly defined invars.lesswith specific values for each size variant (large: 28px, medium: 24px, small: 14px, mini: 12px, default: 18px), I can now generate the final response.CSS custom properties are properly defined ✓
The implementation is complete and correct. All required
--tv-Button-size-icon-font-size*custom properties are properly defined invars.lesswith appropriate size gradations that follow a consistent scaling pattern (28px → 12px), which aligns well with typical design system specifications.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all required CSS custom properties are defined in vars.less # Test: Search for the icon size custom properties in vars.less rg --type less "tv-Button-size-icon-font-size" "packages/theme/src/button/vars.less" # Test: Check if there are any other icon size related properties we should consider rg --type less "icon.*size" "packages/theme/src/button/vars.less"Length of output: 669
packages/theme/src/button/vars.less (3)
38-39: LGTM! Well-documented default icon size variable.The default icon size of 18px is appropriate for standard button sizes and follows common UI design patterns.
58-59: LGTM! Consistent size progression for medium, small, and mini icons.The size progression (24px → 14px → 12px) follows a logical scale and maintains good proportions:
- Medium (24px): Good intermediate size between default and large
- Small (14px): Appropriate reduction from default size
- Mini (12px): Suitable for compact UI elements
Also applies to: 68-69, 78-79
48-49: Verify the large icon size proportions.While 28px is a valid size for large icons, it's a significant jump from the default size (18px). Consider testing this size in various contexts to ensure it doesn't appear disproportionate.
Consider the following aspects:
- Visual balance when used alongside text
- Alignment with other large UI elements
- Rendering quality at this size
Run this script to analyze the usage context:
✅ Verification successful
Let me try a different approach to verify the icon size proportions by examining the button-related style definitions and other icon size declarations.
This will help us understand:
- The relationship between different icon sizes
- The context of large button styles
- The overall icon size system in the theme
Based on the search results, I can now make an informed assessment about the icon size proportions in the button component. Let me analyze the relevant findings:
The icon size system follows a consistent scale:
- Default button icon size: 18px
- Medium button icon size: 24px
- Large button icon size: 28px
- Small button icon size: 14px
- Mini button icon size: 12px
The progression from default (18px) to large (28px) is proportional and aligns with other large icons in the system. For example:
- Upload picture card icon: 24px
- Top box icon: 24px
- Modal close icon: 24px
- Notify status icon: 24px
The 28px size for large button icons is appropriate because:
- It maintains the same 10px increment from medium (24px) as medium has from default (18px)
- It's proportionally consistent with other large UI elements in the system
- It aligns with the design system's size scale pattern
Final Response:
The large icon size (28px) follows the design system's proportions
The 28px size for large button icons is well-proportioned within the design system. It follows a consistent scaling pattern (12px → 14px → 18px → 24px → 28px) and aligns with other large UI elements. The 10px increment from medium to large matches the 6px increment from default to medium, maintaining visual harmony.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all button components with large size and icon-only variants # to verify the context where this size will be applied # Search for large button declarations rg -A 3 'size="large".*only-icon|is-only-icon.*size="large"'Length of output: 62
Script:
#!/bin/bash # Search for all button-related icon size definitions rg -g '*.less' -g '*.css' -g '*.scss' 'icon.*size|font-size' packages/theme/src/ # Search for large button style definitions rg -g '*.less' -g '*.css' -g '*.scss' 'large.*button|button.*large' packages/theme/src/ # Look for any existing icon size variables or declarations rg -g '*.less' -g '*.css' -g '*.scss' 'icon.*font-size|font-size.*icon' packages/theme/src/Length of output: 93648
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