fix(tag): update tag's size when only icon, update one place of button #2440
Conversation
WalkthroughThe changes in this pull request primarily involve the introduction of several Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
examples/sites/demos/pc/app/tag/size.vue (1)
Line range hint
1-35: Consider enhancing the demo documentation.Since this demo specifically addresses the tag sizing behavior with icons, it would be helpful to add comments or documentation explaining:
- The expected sizing behavior of icon-only tags
- Any size-specific considerations when using only icons
- The relationship between icon and text sizes
Consider adding a comment block above the template:
<!-- This demo showcases tag sizing variants: - Standard tags with text in different sizes (medium, default, small, mini) - Icon-only tags which maintain consistent sizing across variants Note: Icon-only tags use the `only-icon` prop to ensure proper sizing and spacing. -->examples/sites/demos/pc/app/tag/webdoc/tag.js (1)
14-15: Consider adding size adjustment details to the documentation.Since the PR specifically addresses tag sizing when only icons are present, consider adding information about how the size adjusts in this case. This would help users understand the component's behavior better.
Example addition:
通过 <code>only-icon</code> 属性，设置标签只有图标。 +标签大小会根据不同尺寸自动调整。 Use the <code>only-icon</code> property to set the label to only ICONS. +The tag size will automatically adjust based on different dimensions.Also applies to: 19-20
packages/theme/src/tag/vars.less (2)
27-30: LGTM! Consider enhancing documentation.The variables for mini size icon-only mode are well-structured and follow consistent naming patterns.
Consider adding a comment explaining the relationship between these values and the regular mini size values for better maintainability.
Line range hint
27-69: Overall implementation looks solid.The new icon-only variables:
- Follow consistent naming patterns
- Maintain clear size progression
- Are well-documented
- Integrate seamlessly with existing variables
Consider documenting the rationale behind the size progression in a comment block to help future maintainers understand the design decisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- examples/sites/demos/pc/app/tag/size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/tag/size.vue (1 hunks)
- examples/sites/demos/pc/app/tag/webdoc/tag.js (1 hunks)
- packages/theme/src/button/vars.less (1 hunks)
- packages/theme/src/tag/index.less (1 hunks)
- packages/theme/src/tag/vars.less (4 hunks)
🔇 Additional comments (9)
examples/sites/demos/pc/app/tag/size-composition-api.vue (2)
7-11: LGTM! Well-structured demo implementation.The new tag examples effectively demonstrate the
only-iconfunctionality across different sizes. The consistent use of the same icon type helps users focus on the size variations.
17-19: Verify icon component registration.Let's verify how the icon component is registered to ensure we're using the correct approach.
✅ Verification successful
The current icon implementation is correct
The code is following the correct pattern for icon usage in the Vue 3 Composition API context:
- The icon is properly imported from '@opentiny/vue-icon'
- The icon component is correctly initialized using
IconHeartempty()- The component is used appropriately in the template with the
tiny-icon-heartemptytag- This pattern is consistently used across multiple files in the codebase (basic-usage-composition-api.vue, size-composition-api.vue)
No global registration is needed as the icon is properly scoped within the component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for global registration of tiny-icon-heartempty rg -l "app.component.*tiny-icon-heartempty" # Search for component registration patterns ast-grep --pattern "createApp($_).component('tiny-icon-heartempty', $_)"Length of output: 192
Script:
#!/bin/bash # Let's check the actual usage pattern in the file and related files rg -p "IconHeartempty" --type vue # Check for any global component registration patterns in the codebase rg -p "app\.component\(" --type vue --type js --type ts # Look for any icon registration patterns ast-grep --pattern "import { IconHeartempty } from '@opentiny/vue-icon'"Length of output: 228
Script:
#!/bin/bash # Let's check the actual usage pattern in the file and related files rg -p "IconHeartempty" # Check for any global component registration patterns in the codebase rg -p "app\.component\(" # Look at the content of the file in question cat "examples/sites/demos/pc/app/tag/size-composition-api.vue"Length of output: 2347
examples/sites/demos/pc/app/tag/size.vue (1)
7-11: LGTM! Well-structured demo implementation.The new icon-only tag examples are well organized and consistently demonstrate all available sizes, matching the pattern of the text-based tags above. The line breaks provide clear visual separation between the different tag variations.
examples/sites/demos/pc/app/tag/webdoc/tag.js (1)
14-15: Documentation updates look good!The added descriptions for the
only-iconproperty are clear, consistent in both languages, and align well with the PR's objective of addressing tag sizing for icon-only cases.Also applies to: 19-20
packages/theme/src/tag/index.less (1)
47-48: LGTM! Well-structured variable declarations.The new variables follow the established naming pattern and are properly scoped within the size mixin.
packages/theme/src/tag/vars.less (3)
53-56: LGTM! Values follow consistent progression.The default size variables maintain logical progression in both font size (16px) and padding (3px).
66-69: Verify medium size increments with design system.The medium size shows larger increments in both font size (16px → 20px) and padding (3px → 5px). While this maintains the visual hierarchy, please verify these values align with the design system's scale.
#!/bin/bash # Search for similar size increments in the theme echo "Searching for similar size patterns in theme files..." rg -i '(16px.*20px|3px.*5px)' packages/theme/src/
40-43: Verify padding consistency across sizes.The font size shows proper progression from mini size (12px → 14px), but the padding remains the same (2px). Is this intentional?
✅ Verification successful
Padding values show intentional progression across sizes
The padding values for icon-only tags follow a clear progression:
- Mini: 2px
- Small: 2px
- Default: 3px
- Medium: 5px
The same padding for mini and small sizes appears intentional as part of the design system's size scale, with values increasing for larger sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing padding patterns in tag styles rg -i 'padding.*2px' packages/theme/src/tag/Length of output: 17246
Script:
#!/bin/bash # Search for all padding variables in tag styles to understand the padding patterns across sizes rg -i 'padding.*px' packages/theme/src/tag/Length of output: 595
packages/theme/src/button/vars.less (1)
69-69: Verify the impact of increased icon size on button components.The icon size for small buttons has been increased from 14px to 16px. While this change aligns with the PR objective to fix tag sizing, please verify that this modification doesn't adversely affect the appearance of small buttons across the application.
Consider adding a comment explaining the rationale for this size adjustment to help future maintainers understand the change:
// 小型按钮内图标的大小 - --tv-Button-size-icon-font-size-small: 16px; + // Increased to 16px to maintain consistent proportions with other sizes (28px -> 24px -> 18px -> 16px -> 12px) + --tv-Button-size-icon-font-size-small: 16px;✅ Verification successful
Based on the search results, I can now generate a final response:
Icon size change appears safe and consistent with usage patterns
The increased icon size (14px to 16px) aligns well with the current usage patterns found in the codebase:
- Small buttons with icons are primarily used in demo examples showing size variations
- The change maintains visual hierarchy in both regular and circle button variants
- The variable is scoped to buttons and doesn't affect other components like Switch or Tag that have their own size definitions
The modification creates a more balanced size progression (28px → 24px → 18px → 16px → 12px) while remaining compatible with existing button implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for components using small button icons to assess impact # Look for button components with size="small" and icons rg -A 5 'size="small".*icon' # Look for direct usage of the LESS variable rg --type less '\-\-tv-Button-size-icon-font-size-small'Length of output: 4107
| import { IconHeartempty } from '@opentiny/vue-icon' | ||
|
|
||
| const TinyIconHeartempty = IconHeartempty() |
There was a problem hiding this comment.
Simplify icon component usage.
The current implementation has a few inconsistencies:
IconHeartempty()initialization is unusual for a Vue component- The initialized
TinyIconHeartemptyisn't used in the template (which usestiny-icon-heartempty)
Consider this simplified approach:
import { TinyTag } from '@opentiny/vue'
import { IconHeartempty } from '@opentiny/vue-icon'
-
-const TinyIconHeartempty = IconHeartempty()Then in the template, either use:
<tiny-tag size="medium" type="success" only-icon>
<icon-heartempty />
</tiny-tag>Or if tiny-icon-heartempty is a registered global component, keep the current template usage but remove the unused import and initialization.
| </div> | ||
| </template> | ||
|
|
||
| <script lang="jsx"> | ||
| import { TinyTag } from '@opentiny/vue' | ||
| import { IconHeartempty } from '@opentiny/vue-icon' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the icon component registration.
While the implementation works, instantiating the icon component during registration (IconHeartempty()) might not be the optimal approach.
Consider registering the component directly:
import { IconHeartempty } from '@opentiny/vue-icon'
export default {
components: {
TinyTag,
- TinyIconHeartempty: IconHeartempty()
+ TinyIconHeartempty: IconHeartempty
}
}This approach:
- Avoids unnecessary component instantiation during registration
- Follows Vue's standard component registration pattern
- May provide better performance characteristics
Also applies to: 21-22
| &.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon { | ||
| padding: e(@icon-py); | ||
|
|
||
| svg { | ||
| font-size: e(@icon-fs); | ||
| margin-right: 0; // 此时不需要右边距了 | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address style inconsistencies and duplication.
There are a few issues to address:
- Inconsistent specificity between root level (triple class) and size mixin (double class)
- Duplicated
margin-right: 0rule that exists at both levels
Consider consolidating the common styles:
// At root level
-&.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon {
+&&.@{tag-prefix-cls}--only-icon {
padding: var(--tv-Tag-only-icon-padding);
svg {
- font-size: var(--tv-Tag-only-icon-font-size);
margin-right: 0; // Move common style here
}
}
// In size mixin
-&.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon {
+&&.@{tag-prefix-cls}--only-icon {
padding: e(@icon-py);
svg {
font-size: e(@icon-fs);
- margin-right: 0; // Remove duplicate
}
}Committable suggestion was skipped due to low confidence.
| &.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon { | ||
| padding: var(--tv-Tag-only-icon-padding); | ||
|
|
||
| svg { | ||
| font-size: var(--tv-Tag-only-icon-font-size); | ||
| margin-right: 0; // 此时不需要右边距了 | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more maintainable specificity approach.
While the triple class selector .@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon achieves high specificity, it's not a maintainable pattern. Consider using a more semantic approach.
- &.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon {
+ // Use a single class with higher specificity selector
+ &&.@{tag-prefix-cls}--only-icon {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon.@{tag-prefix-cls}--only-icon { | |
| padding: var(--tv-Tag-only-icon-padding); | |
| svg { | |
| font-size: var(--tv-Tag-only-icon-font-size); | |
| margin-right: 0; // 此时不需要右边距了 | |
| } | |
| } | |
| &&.@{tag-prefix-cls}--only-icon { | |
| padding: var(--tv-Tag-only-icon-padding); | |
| svg { | |
| font-size: var(--tv-Tag-only-icon-font-size); | |
| margin-right: 0; // 此时不需要右边距了 | |
| } | |
| } |
* fix(build): fix build error * fix(docs): 增加上传文件类型说明 (#2439) * fix(tag): update tag's size when only icon, update one place of button's icon size (#2440) * fix(date-picker): [date-picker,select] fix radius bugs for month-table (#2442) * docs(site): fix demos and test e2e (#2444) * test(site): fix icon not correct test e2e (#2445) --------- Co-authored-by: chenxi-20 <[email protected]> Co-authored-by: 申君健 <[email protected]> Co-authored-by: MomoPoppy <[email protected]> Co-authored-by: gimmyhehe <[email protected]>
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
增加tag的only icon时， 不同尺寸时的大小
Summary by CodeRabbit
Release Notes
New Features
<tiny-tag>components that display only icons in various sizes (medium, small, mini).Bug Fixes
Documentation
only-iconproperty.Style