fix(card): replace card-group,card-template components css-vars #2333
Conversation
WalkthroughThe changes involve the removal of several LESS files related to card components, including styles and variable definitions. Specifically, the 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 (
|
|
[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: 3
🧹 Outside diff range and nitpick comments (3)
packages/theme/src/card-template/index.less (2)
40-43: LGTM: Consistent variable renaming for title styling, with a minor suggestion.The changes to title styling variables (font weight, font size, and text color) align with the new naming convention. However, there's a potential inconsistency:
Consider replacing the hardcoded color value on line 42 with a CSS variable for better theming consistency:
- color: #333; + color: var(--tv-CardTemplate-header-title-text-color);This change would make line 43 redundant, so you could remove it if you make this adjustment.
61-62: LGTM: Consistent variable renaming, with a suggestion for clarity.The changes from
--ti-card-header-heightto--tv-CardTemplate-header-heightfor both height and line-height of list items are consistent with the new naming convention.Consider creating a separate variable for list item height and line-height for better clarity:
- height: var(--tv-CardTemplate-header-height); - line-height: var(--tv-CardTemplate-header-height); + height: var(--tv-CardTemplate-list-item-height); + line-height: var(--tv-CardTemplate-list-item-height);This would make it clearer that these properties are specific to list items rather than the header.
packages/theme/src/card-template/vars.less (1)
18-19: Replace hardcoded values with CSS variables for consistencyThe following properties are assigned hardcoded values:
- Line 18:
--tv-CardTemplate-tool-active-icon-color: #595959;- Line 19:
--tv-CardTemplate-tool-selector-height: 40px;- Line 26:
--tv-CardTemplate-header-border-color: #999999;For better theming consistency and easier customization, consider using existing CSS variables or defining new ones to replace these hardcoded values.
Also applies to: 26-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/theme/src/card-group/index.less (0 hunks)
- packages/theme/src/card-group/vars.less (0 hunks)
- packages/theme/src/card-item/index.less (0 hunks)
- packages/theme/src/card-item/vars.less (0 hunks)
- packages/theme/src/card-layout/index.less (0 hunks)
- packages/theme/src/card-layout/vars.less (0 hunks)
- packages/theme/src/card-template/index.less (5 hunks)
- packages/theme/src/card-template/vars.less (1 hunks)
- packages/theme/src/index.less (1 hunks)
💤 Files with no reviewable changes (6)
- packages/theme/src/card-group/index.less
- packages/theme/src/card-group/vars.less
- packages/theme/src/card-item/index.less
- packages/theme/src/card-item/vars.less
- packages/theme/src/card-layout/index.less
- packages/theme/src/card-layout/vars.less
🧰 Additional context used
🔇 Additional comments (12)
packages/theme/src/card-template/index.less (10)
23-23: LGTM: Consistent variable renaming.The change from
--ti-card-bg-colorto--tv-CardTemplate-bg-coloraligns with the new naming convention, making it more specific to the CardTemplate component.
31-32: LGTM: Consistent variable renaming for header dimensions.The changes from
--ti-card-header-heightto--tv-CardTemplate-header-heightfor both height and line-height are consistent with the new naming convention and maintain proper spacing.
35-35: LGTM: Consistent variable renaming for header border.The change from
--ti-card-header-border-colorto--tv-CardTemplate-header-border-coloraligns with the new naming convention and is specific to the CardTemplate component.
51-51: LGTM: Consistent variable renaming for tool icon size.The change from
--ti-card-tool-icon-sizeto--tv-CardTemplate-tool-icon-sizealigns with the new naming convention and is specific to the CardTemplate component.
68-68: LGTM: Consistent variable renaming for tool icon color.The change from
--ti-card-tool-icon-colorto--tv-CardTemplate-tool-icon-coloraligns with the new naming convention and is specific to the CardTemplate component.
73-80: LGTM: Consistent variable renaming for tool icon states.The changes to tool icon color variables for default, hover, and active states align with the new naming convention and are specific to the CardTemplate component.
92-99: LGTM: Consistent variable renaming for 'more' section with good practices.The changes to variables for the 'more' section (top positioning, border color, background color, and text color) align with the new naming convention and are specific to the CardTemplate component. The use of a fallback value in the
calc()function on line 92 is a good practice for ensuring layout stability.
119-122: LGTM: Consistent variable renaming for 'more' section list items.The changes to variables for the 'more' section list items (height, line height, and text color) align with the new naming convention and are specific to the CardTemplate component.
Line range hint
127-150: LGTM: Comprehensive and consistent variable renaming for list item states.The changes to variables for the 'more' section list item states (default, hover, active) are consistent with the new naming convention and specific to the CardTemplate component. The styling is comprehensive, covering text colors, background colors, and icon colors for each state.
Line range hint
1-160: Overall assessment: Approved with minor suggestions.The changes in this file consistently apply the new naming convention for CSS variables, improving the specificity to the CardTemplate component. The refactoring is well-executed, maintaining the existing functionality while enhancing the theming capabilities.
A few minor suggestions were made:
- Consider using a CSS variable for the hardcoded color value on line 42.
- Evaluate the use of header height variables for list items (lines 61-62) for potential clarity improvements.
These suggestions are optional and do not impede the overall quality and consistency of the changes. The PR can be approved from the perspective of this file.
packages/theme/src/index.less (2)
Line range hint
1-170: Summary of changes in packages/theme/src/index.lessThe changes in this file partially align with the PR objectives:
- Removal of card-item and card-layout imports is consistent with the goal of refactoring card components.
- The addition of year-table import needs clarification as it's not mentioned in the PR objectives.
- There are no visible changes to card-group and card-template imports, which is unexpected given the PR objectives.
Please provide more context on these points to ensure the changes are fully aligned with the PR's goals.
Line range hint
1-170: Inquiry about card-group and card-template refactoring.The PR objectives mention replacing CSS variables for card-group and card-template components. However, there are no visible changes to their imports in this file. Can you please provide an update on the status of this refactoring?
#!/bin/bash # Description: Check for changes in card-group and card-template components # Test: Search for changes in card-group and card-template files git diff origin/main -- '**/card-group/**' '**/card-template/**'
| @import './year-table/index.less'; |
There was a problem hiding this comment.
💡 Codebase verification
The addition of @import './year-table/index.less'; appears unrelated to the PR's objective of refactoring card components. Please clarify the necessity of this import.
🔗 Analysis chain
Verify the addition of year-table import.
The addition of @import './year-table/index.less'; seems unrelated to the PR objectives of refactoring card components. Can you please clarify the reason for this addition?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if year-table component is used in the codebase
# Test: Search for year-table usage
rg -i 'year-table|yeartable' --type vue --type js
Length of output: 100
Script:
#!/bin/bash
# Description: Search for usage of 'year-table' component in the codebase
rg -i 'year-table|yeartable' --type vue --type js --type jsx --type ts --type tsx
Length of output: 111
Script:
#!/bin/bash
# Description: Search for usage of 'year-table' component in the codebase using file extensions
rg -i 'year-table|yeartable' --glob '*.vue' --glob '*.js' --glob '*.jsx' --glob '*.ts' --glob '*.tsx'
Length of output: 4187
| --ti-card-header-title-font-weight: var(--ti-common-font-weight-7, bold); | ||
| --ti-card-header-title-font-size: var(--ti-common-font-size-1, 14px); | ||
| --ti-card-header-title-text-color: var(--ti-common-color-info-normal, #191919); | ||
| .inject-CardTemplate-vars() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming the mixin function for consistency
The mixin .inject-CardTemplate-vars() uses PascalCase in its name. For consistency with LESS mixin naming conventions in the codebase, consider renaming it to .inject-card-template-vars(), using lowercase letters and hyphens.
| --tv-CardTemplate-bg-color: var(--tv-color-bg-secondary, #ffffff); | ||
| --tv-CardTemplate-tool-icon-size: var(--tv-icon-size); | ||
| --tv-CardTemplate-tool-icon-color: var(--tv-color-icon-control, #191919); | ||
| --tv-CardTemplate-tool-hover-icon-color: #c2c2c2; // 无对应的正确变量 |
There was a problem hiding this comment.
Replace hardcoded color value with a CSS variable
At line 17, --tv-CardTemplate-tool-hover-icon-color is assigned a hardcoded color value #c2c2c2. The comment indicates that there is no corresponding correct variable (// 无对应的正确变量). To maintain theming flexibility and consistency, consider defining a new CSS variable or using an existing one to replace the hardcoded color.
Would you like assistance in identifying or creating an appropriate CSS variable for this color?
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
重构 card-group,card-template 的 css 变量
移除 card-item card-layout 的样式文件。好像没地方引用
Summary by CodeRabbit