refactor(tree): [tree] refactor tree theme #2319
Conversation
WalkthroughThe changes in this pull request involve a comprehensive update to the theming conventions for tree components in the CSS files 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: 1
🧹 Outside diff range and nitpick comments (7)
packages/theme/src/tree/vars.less (4)
13-33: Approve function renaming and variable prefix changes.The changes in this section improve consistency and follow a new naming convention:
- Function renamed to
.inject-Tree-vars(), which is more descriptive.- CSS variable prefix changed from
--ti-to--tv-consistently.- Updated references to use standardized spacing variables (e.g.,
--tv-space-xl).These changes enhance code readability and maintain a consistent naming scheme.
Consider updating the documentation to reflect these naming changes, especially if there are any public-facing docs or API references.
35-63: Approve node-related variable changes with a suggestion.The changes in this section improve the styling capabilities of the Tree component:
- Consistent renaming with the
--tv-Tree-prefix enhances clarity.- New variables for node content styling (e.g., hover and current states) provide more granular control.
These changes will make it easier to customize the Tree component's appearance.
For the simplified variable on line 33 (
--tv-Tree-empty-min-height: 60px;), consider if this should remain a hard-coded value or if it should reference a design token for better maintainability and consistency across the design system.
66-68: Approve new node styling variables with a suggestion for improvement.The addition of new variables for node padding and highlight background color enhances the styling capabilities of the Tree component.
The comment on line 68 indicates that there's no common variable for the highlight background color (
#f2f2f2). Consider creating a common design token for this color to maintain consistency across the design system. This would allow for easier updates and ensure visual coherence with other components.
70-79: Approve new node content and menu styling variables with a suggestion for improvement.The addition of new variables for node content height, border radius, and menu styling enhances the customization options for the Tree component.
The comment on line 79 indicates that there's no common variable for the menu box shadow. Consider creating a reusable design token for box shadows to maintain consistency across the design system. This would allow for easier updates and ensure visual coherence with other components that use similar shadows.
packages/theme/src/tree/index.less (3)
280-281: RedundantfillandcolorpropertiesBoth
fillandcolorproperties are set tovar(--tv-Tree-node-collapse-icon-color). Verify if both are necessary. If only one affects the icon, consider removing the redundant property for cleaner code.
336-337: Redundantfillandcolorproperties in hover stateIn the hover state, both
fillandcolorproperties are assignedvar(--tv-Tree-node-checked-icon-color). Ensure that both are required for the desired styling. Removing unnecessary properties can simplify the code.
158-159: RedundantcolorandfillpropertiesBoth
colorandfillare set tovar(--tv-Tree-text-color). Confirm if both are needed for the elements being styled. If not, consider omitting one to streamline the CSS.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/tree/index.less (14 hunks)
- packages/theme/src/tree/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/theme/src/tree/vars.less (1)
120-131: Approve Tree popover variable changes.The changes in this section improve consistency and clarity:
- Function renamed to
.inject-Tree-in-Popover-vars(), aligning with the new naming convention.- Consistent renaming of variables with the
--tv-Tree-popover-prefix.These changes enhance code readability and maintain a consistent naming scheme across the Tree component and its associated popover.
packages/theme/src/tree/index.less (4)
23-23: Update method call to.inject-Tree-vars();The change from
.component-css-vars-tree();to.inject-Tree-vars();aligns with the new theming convention and is appropriate.
27-31: CSS variables updated to new naming conventionThe properties for background, color, line-height, padding-top, and padding-bottom now use the new
--tv-prefixed variables, which is consistent with the updated theming structure.
38-38: Variable name consistency checkEnsure that
--tv-Tree-node-checked-icon-coloris the intended variable for the border color. The naming is consistent with the new convention.
397-397: Consistency in margin calculationThe
margin-rightproperty usescalc(var(--tv-Tree-node-label-margin-left) / 2). Ensure that this calculation aligns with the intended design and that the variable name reflects its usage on the right margin.
| margin-right: var(--ti-tree-node-operate-icon-margin-left); | ||
| fill: var(--tv-Tree-node-operate-icon-color); | ||
| font-size: var(--tv-Tree-node-operate-icon-font-size); | ||
| margin-right: var(--tv-Tree-node-operate-icon-margin-left); |
There was a problem hiding this comment.
Possible incorrect variable in margin-right
The margin-right property is using var(--tv-Tree-node-operate-icon-margin-left);, which might be unintended. Consider using var(--tv-Tree-node-operate-icon-margin-right); or verify that the variable name is correct.
Apply this diff to correct the variable:
- margin-right: var(--tv-Tree-node-operate-icon-margin-left);
+ margin-right: var(--tv-Tree-node-operate-icon-margin-right);📝 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.
| margin-right: var(--tv-Tree-node-operate-icon-margin-left); | |
| margin-right: var(--tv-Tree-node-operate-icon-margin-right); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/tree/filter-view.spec.ts (1)
10-10: LGTM! Consider adding a comment explaining the.nth(0)usage.The addition of
.nth(0)to the selector makes it more specific and potentially more robust, especially if there are or could be multiple radio buttons with the name '高亮' (highlight) in the future. This change helps prevent potential test flakiness.Consider adding a brief comment explaining why
.nth(0)is necessary here. For example:// Ensure we select the first 'highlight' radio button in case multiple exist const btnHighlight = preview.getByRole('radio', { name: '高亮' }).nth(0)This will help other developers understand the reasoning behind this specific selector.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- examples/sites/demos/pc/app/tree/filter-view.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
examples/sites/demos/pc/app/tree/filter-view.spec.ts (1)
Line range hint
1-30: Overall, the changes in this file improve test reliability.While this change is not directly related to the tree theme refactoring mentioned in the PR objectives, it contributes to the overall quality of the test suite. The modification makes the selector for the highlight radio button more specific, which can prevent potential issues if the UI structure changes in the future.
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
--tv-).Bug Fixes