fix(icon): [icon] restore title demo #2379
Conversation
WalkthroughThe pull request introduces various modifications across several Vue component files and documentation. Key changes include updating icon component styles with specific class names, simplifying button descriptions in documentation, and adding new Vue components and test cases. The adjustments aim to enhance the clarity of documentation and improve the visual presentation of icon components while also introducing new functionality related to displaying titles. 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 (
|
shenjunjian
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
examples/sites/demos/pc/app/icon/show-title.spec.ts (1)
1-8: Consider adding more comprehensive test casesThe current test only verifies the presence of the title attribute. Consider adding more test cases to verify:
- Title visibility on hover
- Correct icon rendering
- Accessibility attributes
Would you like me to help generate additional test cases to improve coverage?
examples/sites/demos/pc/app/icon/show-title.vue (2)
1-5: Consider internationalizing the title text.The implementation correctly demonstrates how to add tooltips to icons by placing the title attribute on the parent div. However, consider extracting the Chinese text "复制" into a localization system for better maintainability and internationalization support.
- <div title="复制" class="icon-demo"> + <div :title="t('copy')" class="icon-demo">
17-22: Consider using more flexible margin values.The current margin values (20px 83px) seem very specific. Consider using more flexible or relative units, or CSS variables for better maintainability and responsiveness.
.icon-demo .tiny-svg { - margin: 20px 83px; + margin: var(--tiny-icon-demo-margin, 20px); font-size: 24px; }examples/sites/demos/pc/app/icon/basic-usage.vue (1)
30-46: Consider documenting the icon size/color system.The implementation shows a clear size progression (14px → 48px) but color usage is limited to only two icons. Consider:
- Documenting the size progression in comments
- Either applying a consistent color scheme across all icons or documenting why only specific icons have colors
Example documentation:
.icon-demo .tiny-svg { margin: 20px 40px; vertical-align: middle; } +/* Icon size system: + * - Extra small: 14px + * - Small: 20px + * - Medium: 24px + * - Large: 30px + * - Extra large: 48px + */ .icon-demo .icon-shared { font-size: 14px; }examples/sites/demos/pc/app/icon/webdoc/icon.js (1)
12-33: Improve English translation quality in the basic usage documentation.While the Chinese documentation is clear and well-structured, the English translation has some grammatical issues and awkward phrasing, particularly in the common usage examples.
Consider updating the English translation to:
- 1. Import labels in the template. For example, <code> < tiny-shared /> </code> <br> - 2, in the template by <code> < component> The </code> component is introduced. For example, <code> < component :is="tinyShared" /> </code> <br> - 3. Pass in component properties. For example, <code> < tiny-button :icon="tinyShared" > </code> <br> + 1. Use as a tag in the template. For example: <code><tiny-shared /></code><br> + 2. Use with the <code><component></code> element. For example: <code><component :is="tinyShared" /></code><br> + 3. Pass as a component property. For example: <code><tiny-button :icon="tinyShared"></code><br>examples/sites/demos/pc/app/button/webdoc/button.js (1)
50-51: Enhance documentation clarity for icon button usage.The current descriptions could be improved to provide more complete guidance:
- The Chinese description is concise but lacks information about the expected value type for the
iconproperty- The English description is grammatically awkward and could be clearer
Consider this improved version:
- 'zh-CN': '通过 <code>icon</code> 属性，设置按钮展示图标。', - 'en-US': 'Display icons and receive an icon component through the <code>icon</code> settings button' + 'zh-CN': '通过 <code>icon</code> 属性设置按钮的图标组件。', + 'en-US': 'Set the button icon using the <code>icon</code> property.'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- examples/sites/demos/pc/app/button/webdoc/button.js (1 hunks)
- examples/sites/demos/pc/app/icon/basic-usage-composition-api.vue (2 hunks)
- examples/sites/demos/pc/app/icon/basic-usage.spec.js (1 hunks)
- examples/sites/demos/pc/app/icon/basic-usage.vue (2 hunks)
- examples/sites/demos/pc/app/icon/show-title-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/icon/show-title.spec.ts (1 hunks)
- examples/sites/demos/pc/app/icon/show-title.vue (1 hunks)
- examples/sites/demos/pc/app/icon/webdoc/icon.js (1 hunks)
🔇 Additional comments (11)
examples/sites/demos/pc/app/icon/show-title.spec.ts (1)
1-1: LGTM: Clean import statementThe import statement correctly imports the necessary Playwright test utilities.
examples/sites/demos/pc/app/icon/basic-usage.spec.js (2)
11-11: LGTM! Font-size assertion aligns with component updates.The updated font-size assertion correctly reflects the new
.icon-sharedclass styling of14px.
Line range hint
1-13: Consider expanding test coverage.Given that this PR focuses on restoring the title demo functionality and previous width/height checks were removed, consider adding:
- Tests to verify the icon title functionality
- Tests for icon dimensions if they are still part of the component's contract
Here's a suggested expansion of the test:
test('icon basic usage', async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) await page.goto('icon#basic-usage') const demo = page.locator('#basic-usage') const icons = demo.locator('.icon-demo > svg') // Existing font-size check await expect(icons.first()).toHaveCSS('font-size', '14px') // Add title functionality test const firstIcon = icons.first() await expect(firstIcon).toHaveAttribute('title') // Optional: Add dimension checks if still relevant await expect(firstIcon).toHaveCSS('width', '1em') await expect(firstIcon).toHaveCSS('height', '1em') })examples/sites/demos/pc/app/icon/show-title-composition-api.vue (1)
7-11: Verify the necessity of the TinyIconCopy constant.The
TinyIconCopyconstant is created but never explicitly used. The component works because Vue automatically registers components in<script setup>, but this constant appears redundant.examples/sites/demos/pc/app/icon/show-title.vue (1)
7-15: Verify the icon component registration pattern.The immediate invocation of
IconCopy()in the components registration is unusual. Typically, Vue components are registered directly without invocation.Let's verify if this is a consistent pattern across the codebase:
✅ Verification successful
Let me fix the regex syntax and try different approaches to verify this pattern.
Let me try a different approach to search for icon component registrations.
Let me check how other icons from @opentiny/vue-icon are registered to verify if this is a consistent pattern.
Based on the search results, I can now provide a definitive response about the icon component registration pattern.
The immediate invocation of icon components is the correct pattern
The search results show that this is a consistent pattern across the entire codebase. All icon components from
@opentiny/vue-iconare registered with immediate invocation, for example:
IconCopy: IconCopy()IconClose: IconClose()IconSearch: IconSearch()- And hundreds of other similar registrations
This pattern is used consistently in:
- Core components in
packages/vue/src/- Demo examples in
examples/sites/demos/- Both PC and mobile-first implementations
The immediate invocation pattern (
IconCopy()) is not unusual but rather the standard way to register icons in this codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how IconCopy and other icons are registered in components # Test: Search for icon component registrations rg -l "from '@opentiny/vue-icon'" | xargs rg -A 3 "components:\s*{"Length of output: 169
Script:
#!/bin/bash # Search for icon component registrations with proper regex escaping # First find files importing from @opentiny/vue-icon echo "=== Files importing from @opentiny/vue-icon ===" rg "from '@opentiny/vue-icon'" echo -e "\n=== Component registrations in these files ===" # Use simpler pattern matching to find component registrations rg "components:\s*\{" --type vue # Look specifically for IconCopy registrations echo -e "\n=== IconCopy registrations ===" rg "IconCopy[^}]+" --type vueLength of output: 75513
Script:
#!/bin/bash # Search for icon component registrations patterns echo "=== Icon component registrations ===" rg "components:\s*{[^}]*Icon[^}]*}" -U # Look specifically for IconCopy registrations echo -e "\n=== IconCopy registrations ===" rg "IconCopy\s*:\s*IconCopy\(\)" -ULength of output: 691
Script:
#!/bin/bash # Look for other icon component registrations with immediate invocation echo "=== Other icon registrations with immediate invocation ===" rg "Icon\w+:\s*Icon\w+\(\)" -U # Look for icon component registrations without invocation echo -e "\n=== Icon registrations without invocation ===" rg "Icon\w+:\s*Icon\w+[,}]" -ULength of output: 42568
examples/sites/demos/pc/app/icon/basic-usage.vue (2)
3-7: LGTM! Well-structured icon components with semantic class names.The addition of specific class names for each icon follows good practices by:
- Using consistent naming conventions
- Providing isolated styling hooks
- Improving maintainability
27-29: Verify the margin reduction impact.The margin has been significantly reduced from 83px to 40px, which could affect the layout spacing between icons.
Please confirm if this spacing reduction was intentional and doesn't negatively impact the visual presentation.
examples/sites/demos/pc/app/icon/basic-usage-composition-api.vue (2)
3-7: LGTM! Clean and consistent icon implementation.The template changes follow Vue.js best practices by using specific classes for styling individual icons, making the code more maintainable.
23-25: Verify that the reduced margin provides sufficient spacing.The margin between icons has been reduced from 83px to 40px. While this should still provide adequate spacing, please verify that the icons don't appear too cramped in the demo, especially on smaller screens.
examples/sites/demos/pc/app/icon/webdoc/icon.js (1)
37-50: LGTM! Clear documentation of the title attribute behavior.The new demo clearly documents an important implementation detail about the title attribute behavior and provides a proper workaround. The explanation is clear in both languages and will help developers avoid a common pitfall.
Let's verify if the referenced Vue component exists and implements this behavior:
✅ Verification successful
Let me try to verify the implementation with a different approach.
Implementation matches the documented behavior
The verification confirms that the demo implementation in
show-title.vueperfectly matches the documented behavior. Specifically:
- The title attribute is correctly placed on the parent
divelement (<div title="复制">) rather than on the icon component itself- The implementation serves as a clear example of the workaround described in the documentation
- The presence of related files (spec, composition API version, and documentation in multiple languages) shows this is a well-documented pattern
The review comment's approval is justified as both the documentation and implementation are aligned and effectively communicate this important implementation detail to developers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the show-title.vue demo file fd "show-title.vue" --type f # Check if the implementation follows the documented pattern rg -A 5 "title=" --type vueLength of output: 311
Script:
#!/bin/bash # Let's check the content of the specific icon demo file cat "examples/sites/demos/pc/app/icon/show-title.vue" # Also search for any title-related patterns in icon-related files rg -l "title" examples/sites/demos/pc/app/icon/Length of output: 791
examples/sites/demos/pc/app/button/webdoc/button.js (1)
50-51: Verify alignment with PR objectives.The PR is titled "fix(icon): restore title demo" but these documentation changes don't include any information about the title functionality mentioned in the PR objectives.
Let's check if there are related icon title demos or documentation:
| test('显示title', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling
While the current error handling catches page errors, it might be too strict by expecting all exceptions to be null. This could make the test brittle to unrelated errors.
Consider a more specific error assertion:
- page.on('pageerror', (exception) => expect(exception).toBeNull())
+ page.on('pageerror', (exception) => {
+ console.error('Page error:', exception);
+ expect(exception.message).not.toContain('icon-related error');
+ })Committable suggestion was skipped due to low confidence.
| await page.goto('icon#show-title') | ||
|
|
||
| await expect(page.getByTitle('复制')).toHaveCount(1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify navigation completion and enhance element location strategy
The test could be more robust by:
- Ensuring navigation completes successfully
- Using a more specific locator strategy
- Adding a reasonable timeout for the assertion
Consider this enhanced implementation:
- await page.goto('icon#show-title')
-
- await expect(page.getByTitle('复制')).toHaveCount(1)
+ await page.goto('icon#show-title')
+ await page.waitForLoadState('networkidle')
+
+ const iconElement = page.locator('.tiny-icon[title="复制"]')
+ await expect(iconElement).toBeVisible({ timeout: 5000 })
+ await expect(iconElement).toHaveCount(1)📝 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.
| await page.goto('icon#show-title') | |
| await expect(page.getByTitle('复制')).toHaveCount(1) | |
| await page.goto('icon#show-title') | |
| await page.waitForLoadState('networkidle') | |
| const iconElement = page.locator('.tiny-icon[title="复制"]') | |
| await expect(iconElement).toBeVisible({ timeout: 5000 }) | |
| await expect(iconElement).toHaveCount(1) |
| <template> | ||
| <div title="复制" class="icon-demo"> | ||
| <tiny-icon-copy></tiny-icon-copy> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing i18n for the title text.
The hardcoded Chinese text "复制" should be externalized to support internationalization.
<template>
- <div title="复制" class="icon-demo">
+ <div :title="t('copy')" class="icon-demo">
<tiny-icon-copy></tiny-icon-copy>
</div>
</template>Add to script setup:
import { useI18n } from 'vue-i18n'
const { t } = useI18n()| <style scoped> | ||
| .icon-demo .tiny-svg { | ||
| margin: 20px 83px; | ||
| font-size: 24px; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving style maintainability and responsiveness.
The current styles have some potential improvements:
- The margin values (20px 83px) seem arbitrary and could be standardized
- Using fixed pixel values for font-size may not be responsive
<style scoped>
.icon-demo .tiny-svg {
- margin: 20px 83px;
- font-size: 24px;
+ margin: var(--ti-common-space-4x) var(--ti-common-space-8x);
+ font-size: var(--ti-common-font-size-3);
}
</style>📝 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.
| <style scoped> | |
| .icon-demo .tiny-svg { | |
| margin: 20px 83px; | |
| font-size: 24px; | |
| } | |
| </style> | |
| <style scoped> | |
| .icon-demo .tiny-svg { | |
| margin: var(--ti-common-space-4x) var(--ti-common-space-8x); | |
| font-size: var(--ti-common-font-size-3); | |
| } | |
| </style> |
| .icon-demo .icon-shared { | ||
| font-size: 14px; | ||
| } | ||
| .icon-demo .icon-del { | ||
| font-size: 20px; | ||
| } | ||
| .icon-demo .icon-writing { | ||
| font-size: 24px; | ||
| } | ||
| .icon-demo .icon-ascending { | ||
| font-size: 30px; | ||
| fill: blue; | ||
| } | ||
| .icon-demo .icon-clock-work { | ||
| font-size: 48px; | ||
| fill: red; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using design system color variables.
The icon styles are well-organized with a logical progression of sizes. However, consider using your design system's color variables instead of hardcoded colors like blue and red to maintain consistency with your application's theme.
Example refactor:
.icon-demo .icon-ascending {
font-size: 30px;
- fill: blue;
+ fill: var(--ti-primary-color);
}
.icon-demo .icon-clock-work {
font-size: 48px;
- fill: red;
+ fill: var(--ti-danger-color);
}📝 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.
| .icon-demo .icon-shared { | |
| font-size: 14px; | |
| } | |
| .icon-demo .icon-del { | |
| font-size: 20px; | |
| } | |
| .icon-demo .icon-writing { | |
| font-size: 24px; | |
| } | |
| .icon-demo .icon-ascending { | |
| font-size: 30px; | |
| fill: blue; | |
| } | |
| .icon-demo .icon-clock-work { | |
| font-size: 48px; | |
| fill: red; | |
| } | |
| .icon-demo .icon-shared { | |
| font-size: 14px; | |
| } | |
| .icon-demo .icon-del { | |
| font-size: 20px; | |
| } | |
| .icon-demo .icon-writing { | |
| font-size: 24px; | |
| } | |
| .icon-demo .icon-ascending { | |
| font-size: 30px; | |
| fill: var(--ti-primary-color); | |
| } | |
| .icon-demo .icon-clock-work { | |
| font-size: 48px; | |
| fill: var(--ti-danger-color); | |
| } |
2 participants
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
Release Notes
New Features
<tiny-icon-copy>andIconCopyfor enhanced icon functionality.titleattribute for tooltips.Improvements
iconbutton and icon usage documentation.Bug Fixes