fix(tree): [tree] remove __content-box dom, compact with AUI code #2247
Conversation
WalkthroughThe changes in this pull request focus on enhancing the tree component's API and styling. A new property, 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 (
|
shenjunjian
added
refactoring
|
[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 |
shenjunjian
changed the title
github-actions
bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/theme/src/link-menu/index.less (1)
Old class name
tiny-tree-node__content-boxstill in use.The class name was changed from
.tiny-tree-node__content-boxto.tiny-tree-node__label, but instances of the old class remain in the following file:
examples/sites/src/views/layout/layout.vuePlease update these references to ensure consistency and prevent potential styling issues.
🔗 Analysis chain
Line range hint
149-159: Approve class name change, but verify usage across codebase.The change from
.tiny-tree-node__content-boxto.tiny-tree-node__labelimproves semantic clarity. However, ensure that this class name change is consistently applied across the entire codebase to prevent any styling issues.Run the following script to check for any remaining usage of the old class name:
Let's update the verification script to search for the old class name in
.less,.vue, and.jsfiles explicitly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of the old class name # Test: Search for the old class name. Expect: No results. rg --type less --type vue --type js 'tiny-tree-node__content-box'Length of output: 93
Script:
#!/bin/bash # Description: Check for any remaining usage of the old class name in .less, .vue, and .js files # Search for the old class name in specified file types. Expect: No results. rg 'tiny-tree-node__content-box' --glob '*.less' --glob '*.vue' --glob '*.js'Length of output: 153
packages/vue/src/tree/src/tree-node.vue (3)
Line range hint
168-220: Enhanced child node handlingThe addition of the template for rendering child nodes significantly improves the component's ability to handle nested structures. The use of
collapse-transitionprovides smooth animations, and the conditional rendering based onrenderAfterExpandandstate.expandedcan enhance performance by only rendering child nodes when necessary.Consider extracting the child node rendering logic into a separate component to improve maintainability and reduce the complexity of this component. This would also make it easier to test the child node rendering independently.
Line range hint
304-308: New prop for focus background visibilityThe addition of the
isShowFocusBgprop enhances the component's flexibility by allowing users to control the visibility of the focus background. This is a good improvement for accessibility and customization.Consider adding a comment explaining the purpose of this prop in more detail, including when a user might want to set it to
false. This would improve the self-documentation of the component and make it easier for other developers to understand and use this feature.
Line range hint
309-315: New icon components addedThe addition of new icon components (IconFinish, IconEdit, IconDel, IconPlusSquare) enhances the visual functionality of the tree nodes, providing clear indicators for actions like editing, deleting, and adding nodes. This improves the user interface and interaction capabilities of the component.
Consider using a dynamic import for these icon components to potentially improve the initial load time of the component, especially if not all icons are used in every instance of the tree node. This could be implemented using Vue's
defineAsyncComponentfor lazy loading.packages/theme/src/tree/index.less (1)
Line range hint
340-346: Improved hover effect consistency and code simplificationThe changes to the
.@{tree-node-prefix-cls}__contenthover selector improve the consistency of the hover effect by targeting the content-left and content-right elements directly. This aligns with the removal of the content-box DOM element and simplifies the background color application.However, there's a minor redundancy in the code:
.@{tree-node-prefix-cls}__content-right { background-color: var(--ti-tree-node-content-hover-bg-color); }This block is unnecessary as it's already covered by the previous selector.
Consider removing the redundant block to further simplify the code:
- .@{tree-node-prefix-cls}__content-right { - background-color: var(--ti-tree-node-content-hover-bg-color); - }examples/sites/demos/apis/tree.js (2)
506-517: LGTM: Newshow-checked-markproperty adds useful functionalityThe new
show-checked-markproperty is a valuable addition to the tree component. It allows for better visual feedback when nodes are selected in non-multiple selection mode.Consider adding an example usage in the
pcDemofield to help developers understand how to implement this feature. For example:mode: ['pc'], - pcDemo: '' + pcDemo: 'show-checked-mark'This assumes you have or will create a demo named 'show-checked-mark' to showcase this feature.
Line range hint
1-1180: Suggestion: Consider adding more comprehensive examplesWhile the changes made improve the API documentation, it might be beneficial to enhance the overall documentation with more comprehensive examples. This could include:
- Adding code snippets demonstrating the usage of various properties, especially newly added ones like
show-checked-mark.- Providing examples of common use cases or patterns for the tree component.
- Including cross-references between related properties or methods to help developers understand how different parts of the API work together.
These additions would make the documentation more user-friendly and help developers better understand and utilize the tree component's capabilities.
Would you like assistance in drafting some example code snippets or use cases to include in the documentation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- examples/sites/demos/apis/tree.js (1 hunks)
- packages/theme-saas/src/tree-menu/index.less (1 hunks)
- packages/theme-saas/src/tree/index.less (0 hunks)
- packages/theme/src/link-menu/index.less (2 hunks)
- packages/theme/src/toggle-menu/index.less (1 hunks)
- packages/theme/src/tree-menu/index.less (1 hunks)
- packages/theme/src/tree/index.less (3 hunks)
- packages/theme/src/tree/vars.less (0 hunks)
- packages/vue/src/tree/src/tree-node.vue (3 hunks)
💤 Files with no reviewable changes (2)
- packages/theme-saas/src/tree/index.less
- packages/theme/src/tree/vars.less
🧰 Additional context used
🔇 Additional comments (8)
packages/theme/src/tree-menu/index.less (1)
195-197: LGTM! Verify hover effect after class renaming.The change from
__content-boxto__labelaligns with the PR objective of removing the__content-boxDOM element. This modification enhances consistency with AUI code as intended.Please verify that the hover effect on tree nodes still works as expected after this class renaming. You can use the following steps:
- Build and run the component in a test environment.
- Navigate through the tree menu and hover over various nodes.
- Ensure that the visual feedback on hover is consistent and meets design requirements.
If you notice any unexpected behavior, please update the styling accordingly.
packages/theme-saas/src/tree-menu/index.less (1)
167-169: Approve: Improved class naming and layout.The renaming of
.@{tree-node-prefix-cls}__content-boxto.@{tree-node-prefix-cls}__labelenhances the semantic clarity of the code. Theflex-1property ensures the label fills available space, which is appropriate for tree node layouts.packages/vue/src/tree/src/tree-node.vue (2)
48-48: Improved code documentationThe added comment provides a clear explanation of the structure for the current node's row. This enhances code readability and helps other developers understand the layout quickly.
132-134: Improved template structureThe replacement of the
<div>element with a<template>element and the restructuring of the conditional rendering improve the code organization and reduce unnecessary DOM elements. This change enhances both the performance and readability of the component.packages/theme/src/tree/index.less (3)
Line range hint
94-98: Improved highlighting specificity for current tree nodeThe changes to the
.@{tree-prefix-cls}--highlight-currentselector improve the specificity of the highlighting behavior. By targeting the.@{tree-node-prefix-cls}__labeland.@{tree-node-prefix-cls}__content-rightelements directly, instead of the previously used content-box, this change aligns with the PR objective of removing the __content-box DOM. This approach provides better control over the highlighting and ensures consistency with the AUI code.
Line range hint
355-359: Improved handling of long labels in tree nodesThe addition of the
.@{tree-node-prefix-cls}__labelselector with overflow handling properties is a good improvement. This ensures that long labels in tree nodes don't break the layout and are truncated with an ellipsis when necessary, enhancing the overall appearance and usability of the tree component.
Line range hint
1-524: Summary of changesThe modifications to the tree component styles in this file successfully achieve the PR objective of removing the __content-box DOM and ensuring consistency with AUI code. The changes improve the specificity of node highlighting, enhance hover effect consistency, and add better handling for long labels. These updates contribute to a more maintainable and efficient styling structure for the tree component.
A minor optimization was suggested to remove a redundant style block, but overall, the changes are well-implemented and improve the component's styling.
examples/sites/demos/apis/tree.js (1)
500-501: LGTM: Improved formatting forhighlight-querydescriptionThe removal of the line break in the Chinese description improves readability without changing the meaning. This is a good cleanup.
| :not(.show-checkbox) > .@{tree-node-prefix-cls}__content .@{tree-node-prefix-cls}__label, | ||
| .@{tree-node-prefix-cls}__content-right { |
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: Remaining __content-box usage detected.
An occurrence of __content-box remains in examples/sites/src/views/layout/layout.vue, indicating that the removal may be incomplete. Please address this to ensure consistent styling and complete removal across the codebase.
🔗 Analysis chain
LGTM! Verify visual consistency and check for side effects.
The changes align with the PR objective of removing the __content-box DOM element. The new selector targets more specific elements (__label and __content-right), which should lead to more precise styling.
Please verify the following:
- Ensure that the hover effects still work as expected for all tree node elements.
- Check if this change affects any other components or styles that might have depended on the old selector.
- Consider adding before/after screenshots to the PR description to demonstrate that the visual appearance remains consistent.
To help with verification, you can run the following command to check for any other occurrences of __content-box in the codebase:
This will help ensure that the removal is complete and consistent across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of __content-box
rg --type less --type vue --type js '__content-box'
Length of output: 79
Script:
#!/bin/bash
# Search for any remaining occurrences of __content-box in .less, .vue, and .js files
rg '__content-box' --glob '*.less' --glob '*.vue' --glob '*.js'
Length of output: 139
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
show-checked-markproperty to display a checkmark next to selected nodes.Improvements
Bug Fixes
highlight-queryproperty description for improved clarity.Chores