feat(link-menu): [link-menu]modify smb theme #2210
Conversation
WalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
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 (
|
![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 (12)
packages/vue/src/link-menu/src/index.ts (5)
64-64: Consider adding documentation and type specification forexpandIcon.The
expandIconprop has been added, but it lacks documentation and specific type information. To improve developer experience and prevent potential misuse:
- Add JSDoc comments explaining the expected structure of the icon object.
- Consider using TypeScript to define a more specific type instead of just
Object.- If applicable, add a validator function to ensure the passed object has the required properties.
Example improvement:
/** * Custom expand icon for the menu. * @type {Vue.Component | { name: string, props?: Object }} */ expandIcon: { type: Object as PropType<Vue.Component | { name: string, props?: Object }>, default: () => ({}) }
65-65: Consider adding documentation and type specification forshrinkIcon.Similar to the
expandIconprop, theshrinkIconprop lacks documentation and specific type information. To maintain consistency and improve usability:
- Add JSDoc comments explaining the expected structure of the icon object.
- Use TypeScript to define a more specific type instead of just
Object.- If applicable, add a validator function to ensure the passed object has the required properties.
Example improvement:
/** * Custom shrink icon for the menu. * @type {Vue.Component | { name: string, props?: Object }} */ shrinkIcon: { type: Object as PropType<Vue.Component | { name: string, props?: Object }>, default: () => ({}) }
66-69: EnhanceshrinkIconColorprop with validation and meaningful default.The
shrinkIconColorprop has been added, but there are a few points to consider:
- The default empty string might not be meaningful. Consider using a specific color as default.
- There's no validation to ensure a valid color value is provided.
- Documentation about accepted color formats (hex, rgb, named colors) would be helpful.
Consider updating the prop definition as follows:
/** * Color of the shrink icon. Accepts CSS color values. * @type {string} */ shrinkIconColor: { type: String, default: '#000000', // or any other appropriate default color validator: (value: string) => { // Basic validation for hex, rgb, rgba, and named colors return /^(#|rgb|rgba|hsl|hsla|[a-z]+)/.test(value) } }
70-72: EnhanceexpandIconColorprop with validation and meaningful default.The
expandIconColorprop has been added with similar characteristics toshrinkIconColor. Consider the following improvements:
- Replace the default empty string with a meaningful color value.
- Add validation to ensure a valid color value is provided.
- Include documentation about accepted color formats.
Consider updating the prop definition as follows:
/** * Color of the expand icon. Accepts CSS color values. * @type {string} */ expandIconColor: { type: String, default: '#000000', // or any other appropriate default color validator: (value: string) => { // Basic validation for hex, rgb, rgba, and named colors return /^(#|rgb|rgba|hsl|hsla|[a-z]+)/.test(value) } }
63-72: Overall assessment of new icon-related propsThe addition of
expandIcon,shrinkIcon,shrinkIconColor, andexpandIconColorprops enhances the customization capabilities of the LinkMenu component. These changes allow for more flexible theming and appearance control. However, there are a few points to consider:
- Ensure that the component's template and logic have been updated to utilize these new props effectively.
- Update the component's documentation to reflect these new customization options.
- Consider adding examples in the component's documentation or storybook to demonstrate the usage of these new props.
- The PR objectives mentioned modifying the SMB theme, but these changes seem more general. Clarify if there are any SMB-specific changes or if this is intentional.
To fully implement this feature:
- Update the component's template to use the new icon props.
- Implement logic to apply the color props to the respective icons.
- Add unit tests for the new props and their effects on the component's rendering.
- Update any relevant documentation or examples to showcase these new customization options.
examples/sites/demos/pc/app/link-menu/data-resource-composition-api.vue (1)
2-11: LGTM! Consider using CSS custom properties for icon colors.The new props for expand and shrink icons, along with their colors, enhance the customization options for the TinyLinkMenu component. This is a good improvement for flexibility.
Consider using CSS custom properties for the icon colors instead of hardcoding the hex values. This would allow for easier theming and maintenance. For example:
- expandIconColor="#191919" - shrinkIconColor="#191919" + expandIconColor="var(--tiny-link-menu-icon-color, #191919)" + shrinkIconColor="var(--tiny-link-menu-icon-color, #191919)"This way, you can easily change the color for both icons by updating a single CSS custom property.
packages/vue/src/link-menu/src/pc.vue (3)
42-45: LGTM! Consider updating documentation.The addition of
expandIcon,expandIconColor,shrinkIcon, andshrinkIconColorproperties to the<tiny-tree>component enhances customization options for the tree's expand/shrink icons. This aligns well with the PR objective of modifying the SMB theme.As noted in the PR checklist, documentation updates haven't been made for these changes. Consider adding documentation for these new properties to help users understand and utilize them effectively.
80-82: LGTM! Consider using consistent button ordering across the application.The reordering of buttons in the footer, placing "cancel" before "sure", follows a common UI pattern. This change improves user experience by providing a more familiar layout.
To ensure consistency across the application, it might be beneficial to review other dialog boxes or forms and apply the same button order if it differs. This would enhance the overall user experience by maintaining a consistent interface throughout the application.
109-112: LGTM! Consider adding type annotations.The addition of
expandIcon,expandIconColor,shrinkIcon, andshrinkIconColorto the component's props is consistent with the changes made to the<tiny-tree>component. This ensures that these new customization options are properly exposed to users of this component.To improve code maintainability and provide better IDE support, consider adding type annotations for these new properties. For example:
expandIcon: { type: String, default: '' }, expandIconColor: { type: String, default: '' }, shrinkIcon: { type: String, default: '' }, shrinkIconColor: { type: String, default: '' }packages/theme/src/link-menu/index.less (1)
169-174: LGTM! Consider using CSS variables for icon dimensions.The addition of specific styles for
.tree-node-icon svg.tiny-svgis a good improvement, standardizing the size of SVG icons within tree nodes. This change aligns well with the PR objective of modifying the SMB theme for the link-menu component.For better maintainability and consistency with the project's styling approach, consider using CSS variables for the icon dimensions. This would allow for easier theme customization in the future. Here's a suggested modification:
.tree-node-icon { svg.tiny-svg { - width: 16px; - height: 16px; + width: var(--ti-link-menu-tree-icon-size, 16px); + height: var(--ti-link-menu-tree-icon-size, 16px); } }This change introduces a new CSS variable
--ti-link-menu-tree-icon-sizewith a default value of 16px, maintaining the current behavior while allowing for easy customization if needed.examples/sites/demos/apis/link-menu.js (2)
92-105: LGTM! Minor suggestion for English description.The 'shrink-icon' property is well-defined and provides a way to customize the shrink icon, complementing the 'expand-icon' property.
Consider updating the English description for consistency:
- 'en-US': 'Icon indicating shrink' + 'en-US': 'Indicates the shrink icon'This change would make it consistent with the 'expand-icon' description style.
106-119: LGTM! Fix typo in English description.The 'shrink-icon-color' property is well-defined and provides a way to customize the color of the shrink icon, complementing the 'shrink-icon' property.
Please fix the typo in the English description:
- 'en-US': 'Icon indicating shrink color' + 'en-US': 'Indicates the shrink icon color'This change corrects the extra space and makes it consistent with the other descriptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- examples/sites/demos/apis/link-menu.js (1 hunks)
- examples/sites/demos/pc/app/link-menu/data-resource-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/link-menu/data-resource.vue (1 hunks)
- packages/theme/src/link-menu/index.less (1 hunks)
- packages/vue/src/link-menu/src/index.ts (1 hunks)
- packages/vue/src/link-menu/src/pc.vue (3 hunks)
🔇 Additional comments (5)
examples/sites/demos/pc/app/link-menu/data-resource-composition-api.vue (1)
Line range hint
1-93: Overall feedback: Good enhancements, address icon naming, and add tests/docsThe changes to the TinyLinkMenu component enhance its customization options, which is a valuable improvement. However, there are a few areas that need attention:
- The icon naming issue in the script section should be fixed to prevent potential bugs.
- According to the PR objectives, tests for these changes haven't been added. It's important to include tests to ensure the new functionality works as expected and to prevent regressions.
- The PR objectives also mention that documentation updates haven't been made. Please update the component's documentation to reflect these new customization options.
To ensure that the documentation for the TinyLinkMenu component is up to date, please run the following script:
If the script doesn't return any results, it indicates that the documentation needs to be updated to include information about the new props.
examples/sites/demos/pc/app/link-menu/data-resource.vue (1)
Line range hint
1-101: Summary of changes and recommendationsThe modifications to the
<tiny-link-menu>component align with the PR objective of modifying the SMB theme. The changes enhance customization options without introducing breaking changes.Key points:
- New properties for icon colors and custom icons improve visual customization.
- The
keep-selected-nodesproperty might affect component behavior.Recommendations:
- Consider using theme variables for icon colors.
- Implement lazy-loading for icons to potentially improve performance.
- Add comments explaining the purpose of new properties, especially
keep-selected-nodes.- Update tests and documentation as indicated in the PR checklist.
To ensure consistency across the codebase, run the following script:
✅ Verification successful
Verification Successful: Consistent Usage Confirmed
The properties introduced in
<tiny-link-menu>withindata-resource.vueare consistently applied across other components in the codebase. No discrepancies or issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other instances of TinyLinkMenu and verify consistent property usage # Search for other TinyLinkMenu components echo "Searching for other TinyLinkMenu components:" rg --type vue '<tiny-link-menu' -A 10 # Check for other uses of iconPutAway and iconExpand echo "Checking for other uses of iconPutAway and iconExpand:" rg --type vue 'iconPutAway|iconExpand'Length of output: 354
Script:
#!/bin/bash # Description: Check for other instances of TinyLinkMenu and verify consistent property usage within .vue files # Search for other TinyLinkMenu components echo "Searching for other TinyLinkMenu components in .vue files:" rg '<tiny-link-menu' --glob '*.vue' -A 10 # Check for other uses of iconPutAway and iconExpand in .vue files echo "Checking for other uses of iconPutAway and iconExpand in .vue files:" rg 'iconPutAway|iconExpand' --glob '*.vue'Length of output: 13318
packages/vue/src/link-menu/src/pc.vue (1)
Line range hint
1-134: Overall LGTM! Address missing items from PR checklist.The changes in this PR successfully modify the SMB theme for the link-menu component by adding new customization options for the tree icons and improving the button layout. The implementation is sound and aligns well with the PR objectives.
However, there are two important items from the PR checklist that need to be addressed:
- Tests for the changes have not been added.
- Documentation updates for the changes have not been made.
To ensure the quality and maintainability of the code, please add appropriate tests for the new functionality and update the documentation to reflect these changes. Here's a script to check for test files related to this component:
Once these items are addressed, this PR will be ready for merge.
examples/sites/demos/apis/link-menu.js (2)
78-91: LGTM! Color customization for expand icon.The 'expand-icon-color' property is well-defined and provides a way to customize the color of the expansion icon. It complements the 'expand-icon' property nicely.
64-77: LGTM! Verify demo reference.The 'expand-icon' property is well-defined and consistent with other properties. It provides a clear way to customize the expansion icon.
Please verify if 'data-resource' is the correct demo for this new property. If not, update it to the appropriate demo.
#!/bin/bash # Verify if 'data-resource' demo exists and contains relevant code for 'expand-icon' rg --type js 'expand-icon' $(fd -t f 'data-resource' --exec echo {//})
| import { iconPutAway, iconExpand } from '@opentiny/vue-icon' | ||
|
|
||
| const shrinkIcon = iconExpand() | ||
| const expandIcon = iconPutAway() |
There was a problem hiding this comment.
Fix icon naming mismatch
There's a mismatch between the constant names and the icon functions they're assigned to. This could lead to confusion or unexpected behavior.
Please apply the following fix:
import { iconPutAway, iconExpand } from '@opentiny/vue-icon'
-const shrinkIcon = iconExpand()
-const expandIcon = iconPutAway()
+const expandIcon = iconExpand()
+const shrinkIcon = iconPutAway()This ensures that the expand icon is correctly assigned to expandIcon and the shrink icon to shrinkIcon.
📝 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.
| import { iconPutAway, iconExpand } from '@opentiny/vue-icon' | |
| const shrinkIcon = iconExpand() | |
| const expandIcon = iconPutAway() | |
| import { iconPutAway, iconExpand } from '@opentiny/vue-icon' | |
| const expandIcon = iconExpand() | |
| const shrinkIcon = iconPutAway() |
| </template> | ||
|
|
||
| <script lang="jsx"> | ||
| import { LinkMenu } from '@opentiny/vue' | ||
| import { iconPutAway, iconExpand } from '@opentiny/vue-icon' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Addition of custom icons for expand and shrink actions
The changes introduce custom icons for expand and shrink actions, enhancing the visual customization of the component.
However, there's room for improvement:
- Consider lazy-loading the icons to improve performance:
-import { iconPutAway, iconExpand } from '@opentiny/vue-icon'
+import { defineAsyncComponent } from 'vue'
export default {
// ...
data() {
return {
- shrinkIcon: iconExpand(),
- expandIcon: iconPutAway(),
+ shrinkIcon: defineAsyncComponent(() => import('@opentiny/vue-icon').then(m => m.iconExpand)),
+ expandIcon: defineAsyncComponent(() => import('@opentiny/vue-icon').then(m => m.iconPutAway)),
// ...
}
}
}This approach will load the icons only when they're needed, potentially improving initial load time.
- If the icons are used elsewhere in the component, consider moving them to computed properties for better reusability and to avoid unnecessary re-renders.
Also applies to: 24-25
| <tiny-link-menu | ||
| :data="datas" | ||
| title="收藏夹菜单" | ||
| wrap | ||
| expandIconColor="#191919" | ||
| shrinkIconColor="#191919" | ||
| :expand-icon="expandIcon" | ||
| :shrink-icon="shrinkIcon" | ||
| :keep-selected-nodes="false" | ||
| ></tiny-link-menu> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhancement of <tiny-link-menu> component with new properties
The changes improve the customization options for the link menu:
- New icon color properties allow for theme consistency.
- Dynamic binding of expand and shrink icons increases flexibility.
- The
keep-selected-nodesproperty is set to false, which might affect the component's behavior.
Consider the following suggestions:
- Instead of hardcoding icon colors, consider using theme variables or making them dynamic:
- expandIconColor="#191919"
- shrinkIconColor="#191919"
+ :expandIconColor="theme.iconColor"
+ :shrinkIconColor="theme.iconColor"- Add a comment explaining the purpose of
keep-selected-nodesbeing set to false.
Committable suggestion was skipped due to low confidence.
| { | ||
| name: 'expand-icon', | ||
| type: 'Component', | ||
| defaultValue: '', | ||
| meta: { | ||
| stable: '3.19.0' | ||
| }, | ||
| desc: { | ||
| 'zh-CN': '指示展开的图标', | ||
| 'en-US': 'Indicates the expanded icon' | ||
| }, | ||
| mode: ['pc'], | ||
| pcDemo: 'data-resource' | ||
| }, | ||
| { | ||
| name: 'expand-icon-color', | ||
| type: 'string', | ||
| defaultValue: '', | ||
| meta: { | ||
| stable: '3.19.0' | ||
| }, | ||
| desc: { | ||
| 'zh-CN': '指示展开的图标色', | ||
| 'en-US': 'Indicates the expanded icon color' | ||
| }, | ||
| mode: ['pc'], | ||
| pcDemo: 'data-resource' | ||
| }, | ||
| { | ||
| name: 'shrink-icon', | ||
| type: 'Component', | ||
| defaultValue: '', | ||
| meta: { | ||
| stable: '3.19.0' | ||
| }, | ||
| desc: { | ||
| 'zh-CN': '指示收缩的图标', | ||
| 'en-US': 'Icon indicating shrink' | ||
| }, | ||
| mode: ['pc'], | ||
| pcDemo: 'data-resource' | ||
| }, | ||
| { | ||
| name: 'shrink-icon-color', | ||
| type: 'string', | ||
| defaultValue: '', | ||
| meta: { | ||
| stable: '3.19.0' | ||
| }, | ||
| desc: { | ||
| 'zh-CN': '指示收缩的图标色', | ||
| 'en-US': 'Icon indicating shrink color' | ||
| }, | ||
| mode: ['pc'], | ||
| pcDemo: 'data-resource' | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Incomplete Demo Coverage for New Properties.
The data-resource demo does not include examples for the expand-icon-color and shrink-icon-color properties. To ensure all new customization options are properly demonstrated and tested, please update the demo to incorporate these properties.
- Files to update:
examples/sites/demos/pc/app/link-menu/data-resource.vueexamples/sites/demos/pc/app/link-menu/data-resource-composition-api.vue
🔗 Analysis chain
Summary: New icon customization properties added.
The changes introduce four new properties ('expand-icon', 'expand-icon-color', 'shrink-icon', 'shrink-icon-color') that enhance the customization options for the link-menu component. These additions are well-structured and consistent with the existing code.
Please ensure that the 'data-resource' demo adequately demonstrates all four new properties:
If the demo doesn't cover all new properties, consider updating it or creating a new demo specifically for these icon customization features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if 'data-resource' demo includes examples for all new properties
rg --type js -e 'expand-icon' -e 'expand-icon-color' -e 'shrink-icon' -e 'shrink-icon-color' $(fd -t f 'data-resource' --exec echo {//})
Length of output: 1192
Script:
#!/bin/bash
# Corrected shell script to check if 'data-resource' demo includes examples for all new properties
rg --type js 'expand-icon|expand-icon-color|shrink-icon|shrink-icon-color' $(fd -t f 'data-resource')
Length of output: 1170
| @@ -178,6 +166,13 @@ | |||
| width: calc(100% - 36px); | |||
| } | |||
|
|
|||
| .tree-node-icon { | |||
| svg.tiny-svg { | |||
| width: 16px; | |||
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/theme/src/link-menu/vars.less (3)
15-17: LGTM! Consider adding a comment for the new variable.The changes look good. The variable naming has been updated to follow the new convention, and the values now reference global theme variables. This improves consistency and maintainability.
Consider adding a comment for the new variable on line 17, similar to the existing comments:
// 收藏夹设置字体大小 + // Link menu item font size --tv-Link-menu-item-font-size: var(--tv-font-size-md);
42-43: LGTM! Consider adding an English translation for the comment.The addition of the icon size variable is good and follows the established naming convention.
Consider adding an English translation for the comment to improve accessibility for non-Chinese speaking developers:
// 图标大小 + // Icon size --tv-Link-menu-icon-size: var(--tv-icon-size);
15-43: Overall LGTM! Consider adding English translations for comments.The changes in this file consistently update the variable naming convention from
--ti-to--tv-and reference global theme variables for values. This improves theme consistency and maintainability across the project. The introduction of new variables, such as the icon size, enhances the theme's flexibility.To improve accessibility for non-Chinese speaking developers, consider adding English translations for all comments in the file. This would make the code more inclusive and easier to maintain for a diverse team.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/link-menu/index.less (5 hunks)
- packages/theme/src/link-menu/vars.less (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/link-menu/index.less
🔇 Additional comments (4)
packages/theme/src/link-menu/vars.less (4)
19-23: LGTM! Consistent updates to variable names and values.The changes in this segment maintain consistency with the new naming convention and correctly reference the global theme variables. The existing comments provide helpful context for each variable's purpose.
25-29: LGTM! Consistent updates to input-related variables.The changes in this segment correctly update the variable names and values for input-related properties. The new names follow the established convention, and the values reference appropriate global theme variables.
31-35: LGTM! Consistent updates to border and text color variables.The changes in this segment maintain consistency with the new naming convention and correctly reference the global theme variables. The existing comments provide helpful context for each variable's purpose.
37-41: LGTM! Consistent updates to button-related variables.The changes in this segment correctly update the variable names and values for button-related properties. The new names follow the established convention, and the values reference appropriate global theme variables.
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