refactor(theme): [tooltip] refactor tooltip theme vars #2257
Conversation
WalkthroughThe pull request introduces significant updates to the tooltip component's styling by replacing the existing CSS variable naming convention from Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/tooltip/index.less (1)
89-89: Remove unnecessary blank line for cleaner codeLine 89 contains an extra blank line that isn't necessary. Removing it can improve code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/tooltip/index.less (2 hunks)
- packages/theme/src/tooltip/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
packages/theme/src/tooltip/vars.less (1)
13-79: LGTM!The refactoring of tooltip theme variables from
--ti-to--tv-is consistent and aligns with the project's naming conventions. The use of the.inject-Tooltip-vars()mixin enhances maintainability and promotes a standardized approach to theming.packages/theme/src/tooltip/index.less (8)
20-20: Ensure the new mixin.inject-Tooltip-vars()is properly defined and importedAt line 20, the mixin
.inject-Tooltip-vars();replaces the previous.component-css-vars-tooltip();. Verify that this new mixin is correctly defined in yourtooltip.lessor related mixin files and that it's properly imported to ensure all necessary variables are injected.
30-30: Verify the definition of--tv-Tooltip-content-max-heightLine 30 sets the
max-heightusingvar(--tv-Tooltip-content-max-height);. Confirm that this CSS variable is defined in yourvars.lessfile with the intended value to avoid potential rendering issues.
37-40: Check consistency and definitions of updated CSS variablesLines 37-40 update several style properties to use new CSS variables:
border-radius: var(--tv-Tooltip-popper-border-radius);padding: var(--tv-Tooltip-padding-y) var(--tv-Tooltip-padding-x);font-size: var(--tv-Tooltip-popper-font-size);line-height: var(--tv-Tooltip-popper-font-line-height);Ensure that these variables are consistently named according to your naming conventions and that they are properly defined with appropriate values in your theme variables.
45-45: Confirm the--tv-Tooltip-box-shadowvariable is defined and applied correctlyLine 45 sets the
box-shadowusingvar(--tv-Tooltip-box-shadow);. Verify that this variable is defined in your theme variables and that it produces the desired shadow effect for the tooltip.
92-94: Ensure all state-related CSS variables are properly definedFor the tooltip states
.is-normal,.is-info,.is-error,.is-warning,.is-success, and.is-light, new CSS variables are used:
- Text color variables (e.g.,
var(--tv-Tooltip-popper-normal-text-color))- Background color variables (e.g.,
var(--tv-Tooltip-popper-normal-bg-color))- Border color variables (e.g.,
var(--tv-Tooltip-popper-normal-border-color))Verify that all these variables are defined in
vars.lesswith appropriate values to ensure the tooltip displays correctly in all states.Also applies to: 100-102, 108-110, 121-123, 129-131, 142-144
114-114: Verify the definition of--tv-Tooltip-validate-icon-colorLine 114 sets the
fillproperty usingvar(--tv-Tooltip-validate-icon-color);. Ensure this variable is defined to display the validation icons correctly in error tooltips.
136-137: Check dark mode variables for consistencyIn the
.is-darkclass, the background and text colors use:
background: var(--tv-Tooltip-popper-dark-bg-color);color: var(--tv-Tooltip-popper-dark-text-color);Confirm that these variables align with your dark theme specifications and are defined properly to maintain visual consistency.
142-144: Ensure light theme variables are defined and styled correctlyFor the
.is-lightclass, new CSS variables are introduced:
var(--tv-Tooltip-popper-light-text-color)var(--tv-Tooltip-popper-light-bg-color)var(--tv-Tooltip-popper-light-border-color)Verify that these variables are defined in your theme files and that they provide sufficient contrast for readability in light-themed tooltips.
packages/theme/src/tooltip/vars.less
Outdated
| // 文字提示错误图标色 | ||
| --ti-tooltip-validate-icon-color: var(--ti-common-color-error, #f23030); | ||
| --tv-Tooltip-validate-icon-color: var(--tv-color-error-tex); |
There was a problem hiding this comment.
Fix typo in variable name --tv-color-error-tex
There appears to be a typo in the variable name --tv-color-error-tex; it should likely be --tv-color-error-text to match the naming convention of other color variables.
Apply this diff to correct the typo:
- --tv-Tooltip-validate-icon-color: var(--tv-color-error-tex);
+ --tv-Tooltip-validate-icon-color: var(--tv-color-error-text);📝 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.
| --tv-Tooltip-validate-icon-color: var(--tv-color-error-tex); | |
| --tv-Tooltip-validate-icon-color: var(--tv-color-error-text); |
| width: 12px; | ||
| height: 20px; | ||
| background-color: var(--tv-Tooltip-popper-border-color); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using CSS variables for arrow dimensions
The arrow dimensions are hardcoded:
width: 12px;height: 20px;
To allow for more flexibility and theming options, consider defining these dimensions using CSS variables. This approach enhances customization and consistency across different themes.
Apply this change to define arrow dimensions using variables:
.popper__arrow {
position: absolute;
display: block;
- width: 12px;
- height: 20px;
+ width: var(--tv-Tooltip-arrow-width, 12px);
+ height: var(--tv-Tooltip-arrow-height, 20px);
background-color: var(--tv-Tooltip-popper-border-color);
transform: rotateZ(45deg);
border-radius: 2px;
}📝 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.
| width: 12px; | |
| height: 20px; | |
| background-color: var(--tv-Tooltip-popper-border-color); | |
| width: var(--tv-Tooltip-arrow-width, 12px); | |
| height: var(--tv-Tooltip-arrow-height, 20px); | |
| background-color: var(--tv-Tooltip-popper-border-color); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/tooltip/vars.less (1)
77-79: Improved consistency and added control for tooltip sizing.These final changes enhance the tooltip styling system:
The validate icon color now references a design system variable, improving consistency:
--tv-Tooltip-validate-icon-color: var(--tv-color-error-icon);A new variable for content max height has been added:
--tv-Tooltip-content-max-height: 50vh;The content max height variable is a good addition for controlling tooltip sizing. Consider adding a brief comment explaining its purpose and usage to help other developers understand when and how to adjust this value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/theme/src/tooltip/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
packages/theme/src/tooltip/vars.less (7)
13-21: Improved method naming and variable consistency.The changes in this segment enhance the overall structure and consistency of the tooltip styling:
- The method name change from
.component-css-vars-tooltip()to.inject-Tooltip-vars()better reflects its purpose of injecting variables.- The updated variables now reference a broader design system (e.g.,
--tv-color-border-active,--tv-line-height-number), which improves consistency across the entire UI.These changes will make the code more maintainable and easier to understand.
23-28: Clarify the purpose of "(hide)" comments.The updates to the normal tooltip style variables improve consistency with the new naming convention and design system. However, some variables are marked with "(hide)" comments:
// 文字提示正常文本色(hide) --tv-Tooltip-popper-normal-text-color: var(--tv-color-text); // 弹框正常边框色 (hide) --tv-Tooltip-popper-normal-border-color: var(--tv-Tooltip-popper-normal-bg-color);Could you clarify the purpose of these "(hide)" comments? Are these variables deprecated or not directly used? If they are no longer needed, consider removing them to keep the codebase clean.
30-56: Improved consistency and maintainability for status-specific tooltip styles.The updates to the info, error, warning, and success tooltip style variables significantly enhance the overall design system integration:
- Consistent naming convention: All variables now follow the
--tv-Tooltip-popper-[status]-[property]pattern.- Design system integration: Variables now reference color variables from the broader design system (e.g.,
var(--tv-color-info-bg),var(--tv-color-error-text-white)).These changes will make it easier to maintain a consistent look and feel across different tooltip types and simplify future updates to the design system.
58-68: Clarify the decision for light theme border color.The updates to the dark and light theme tooltip style variables improve consistency with the new naming convention and design system. However, there's an interesting decision for the light theme border color:
--tv-Tooltip-popper-light-border-color: var(--tv-Tooltip-popper-light-bg-color);This effectively removes the border for light theme tooltips by setting it to the same color as the background. Is this intentional? If so, consider adding a comment explaining the design decision to make it clear for future developers.
71-71: Improved consistency for box shadow.The update to the box shadow variable enhances consistency with the overall design system:
--tv-Tooltip-box-shadow: var(--tv-shadow-2-down);This change will ensure that tooltips use a standardized shadow effect, improving visual consistency across the UI.
73-75: Improved naming convention for padding variables.The updates to the padding variables enhance readability and align with common CSS naming conventions:
--tv-Tooltip-padding-y: 12px; --tv-Tooltip-padding-x: 16px;These changes from
padding-verticalandpadding-horizontaltopadding-yandpadding-xmake the code more concise and familiar to developers used to popular CSS frameworks.
13-79: Overall excellent refactoring of tooltip theme variables.This refactoring significantly improves the tooltip styling system:
- Consistent naming convention with the
--tv-prefix and logical variable names.- Improved integration with the broader design system, enhancing visual consistency.
- Better organization of variables for different tooltip types and themes.
- Addition of a content max height variable for improved control over tooltip sizing.
These changes will make the codebase more maintainable and easier to understand. A few minor suggestions were made:
- Clarify the purpose of "(hide)" comments on some variables.
- Explain the design decision for setting the light theme border color to the background color.
- Add a brief comment explaining the purpose and usage of the new content max height variable.
Overall, this is a well-executed refactoring that aligns with best practices for CSS variable management in a design system.
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
Bug Fixes
Refactor