refactor(theme): [drawer] refactor drawer theme vars #2237
Conversation
WalkthroughThe changes in this pull request introduce two new CSS variables for background colors in the Changes
Possibly related PRs
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: 9
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/base/vars.less (1)
313-313: LGTM! Consider adding a comment for clarity.The new variable
--tv-color-mask-bgis well-named and appropriately placed in the background color section. The use of rgba() for a semi-transparent black is perfect for mask/overlay effects.Consider adding a brief comment to explain the intended use of this variable, similar to other variables in this file. For example:
- --tv-color-mask-bg: rgba(0, 0, 0, 30%); // 遮罩层带有透明度的背景色 + --tv-color-mask-bg: rgba(0, 0, 0, 30%); // Semi-transparent background color for overlay/mask effects
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/theme/src/base/vars.less (1 hunks)
- packages/theme/src/drawer/index.less (8 hunks)
- packages/theme/src/drawer/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
packages/theme/src/drawer/vars.less (2)
15-41: Verify all variable references are updated to the new prefixThe variables have been updated from the
--ti-prefix to the--tv-prefix to align with the new design system. Ensure that all references to the old variables (--ti-Drawer-) are updated throughout the codebase to prevent styling inconsistencies.Run the following script to check for any remaining references to old variables:
#!/bin/bash # Description: Check for any remaining references to old Drawer variables. # Test: Search for variables with '--ti-Drawer-' prefix. Expect: No matches. rg --type less '--ti-Drawer-'
13-13: Ensure consistent use of the updated function nameThe function name has been changed from
.component-css-vars-drawer()to.inject-Drawer-vars(). Please verify that all references to the old function name have been updated throughout the codebase to prevent any potential issues.Run the following script to check for any remaining references to the old function name:
✅ Verification successful
All references to
.component-css-vars-drawer()have been successfully updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to `.component-css-vars-drawer()` # Test: Search for the old function name. Expect: No matches. rg --type less '\.component-css-vars-drawer\(\)'Length of output: 538
Script:
#!/bin/bash # Description: Check for any remaining references to `.component-css-vars-drawer()` # Test: Search for the old function name. Expect: No matches. rg --type less '\.component-css-vars-drawer\(\)'Length of output: 50
packages/theme/src/drawer/index.less (6)
7-7: Update to New Mixin Method is AppropriateThe mixin method call has been updated to
.inject-Drawer-vars();, aligning with the new theming system. This change is appropriate and ensures consistency with the updated variable naming convention.
19-19: Variable Name Update is CorrectThe background color for the mask has been updated to use
var(--tv-Drawer-mask-bg-color), which follows the new variable naming convention. This change is correct.
29-30: Background Color and Shadow Variables Updated AppropriatelyThe
background-colorandbox-shadowproperties now usevar(--tv-Drawer-bg-color)andvar(--tv-Drawer-shadow), respectively. These changes align with the new variable naming convention and enhance theming consistency.
60-60: Consistent Use of Border Radius VariablesThe border-radius properties have been updated to use
var(--tv-Drawer-border-radius-size). This ensures consistency and aligns with the new theming variables.Also applies to: 67-67, 74-74
148-153: Well-Defined Close Icon Colors with Hover StateThe close icon's
fillcolor and hover state are set usingvar(--tv-Drawer-close-icon-color)andvar(--tv-Drawer-close-icon-color-hover). This practice allows for easy theming and customization.
169-171: Consistent Use of Typography VariablesThe font size, color, and weight for the title are set using variables
var(--tv-Drawer-head-title-font-size),var(--tv-Drawer-head-title-text-color), andvar(--tv-Drawer-head-title-font-weight). This promotes consistency and theming flexibility.
| --tv-Drawer-min-width: 500px; | ||
| // 最大宽度 | ||
| --ti-drawer-max-width: calc(var(--ti-common-space-10, 10px) * 100); | ||
| --tv-Drawer-max-width: 1000px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing width values for flexibility
The minimum and maximum width variables are now set to fixed pixel values (500px and 1000px). Consider using relative units (like em, rem, or percentages) or referencing global sizing variables to enhance responsiveness and maintainability.
Apply this diff if you decide to use global variables:
- --tv-Drawer-min-width: 500px;
- --tv-Drawer-max-width: 1000px;
+ --tv-Drawer-min-width: var(--tv-size-min-width);
+ --tv-Drawer-max-width: var(--tv-size-max-width);Committable suggestion was skipped due to low confidence.
| margin-left: 32px; | ||
| margin-right: 32px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Using Variables Instead of Fixed Margins
The margin-left and margin-right properties are set to 32px. To enhance flexibility and maintain consistency across the theme, consider using variables instead of fixed pixel values.
You could define a variable in vars.less, such as @drawer-horizontal-margin, and use it here:
margin-left: @drawer-horizontal-margin;
margin-right: @drawer-horizontal-margin;| top: 20px; | ||
| right: 20px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Variables for Close Button Positioning
The top and right properties of the close button are set to fixed values (20px). For better consistency and theming flexibility, consider defining these values as variables.
Define variables in vars.less:
@drawer-close-button-top: 20px;
@drawer-close-button-right: 20px;Then apply them:
top: @drawer-close-button-top;
right: @drawer-close-button-right;| height: 24px; | ||
| width: 24px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Variables for Close Button Size
The height and width of the close button are hardcoded to 24px. To allow for easier adjustments and maintain consistency, consider using a variable.
Define a variable:
@drawer-close-button-size: 24px;Then apply it:
height: @drawer-close-button-size;
width: @drawer-close-button-size;| padding: var(--ti-drawer-header-padding-top) var(--ti-drawer-header-padding-right) | ||
| var(--ti-drawer-header-padding-bottom) var(--ti-drawer-header-padding-left); | ||
| background: var(--ti-drawer-bg-color); | ||
| padding: 32px 0 24px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Fixed Padding with Variables in Header
The header's padding is set to 32px 0 24px. Using variables for padding values enhances maintainability and allows for easier theme adjustments.
Define variables:
@drawer-header-padding-top: 32px;
@drawer-header-padding-bottom: 24px;Then apply them:
padding: @drawer-header-padding-top 0 @drawer-header-padding-bottom;| width: var(--tv-Drawer-help-icon-width-height); | ||
| height: var(--tv-Drawer-help-icon-width-height); | ||
| margin-left: 8px; | ||
| fill: var(--tv-Drawer-help-icon-color); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Variable for Help Icon Margin
While the width, height, and fill are set using variables, the margin-left is hardcoded to 8px. Consider using a variable for the margin to maintain consistency.
Define a variable:
@drawer-help-icon-margin-left: 8px;Then apply it:
margin-left: @drawer-help-icon-margin-left;| @@ -202,30 +197,28 @@ | |||
| .@{drawer-prefix-cls}__body { | |||
| flex: auto; | |||
| overflow: auto; | |||
| padding: var(--ti-drawer-padding-top) var(--ti-drawer-padding-right) var(--ti-drawer-padding-bottom) var(--ti-drawer-padding-left); | |||
| border-bottom: 1px solid var(--ti-drawer-divider-body-border-color); | |||
| padding: 0 32px; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Variables for Body Padding
The padding for the body is set to 0 32px. Consider using a variable for the horizontal padding to maintain consistency.
Define a variable:
@drawer-body-horizontal-padding: 32px;Then apply it:
padding: 0 @drawer-body-horizontal-padding;| justify-content: flex-end; | ||
| padding: 24px 0 32px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Variables for Footer Padding
The footer's padding is set to 24px 0 32px. Replacing fixed values with variables enhances consistency and makes future adjustments easier.
Define variables:
@drawer-footer-padding-top: 24px;
@drawer-footer-padding-bottom: 32px;Then apply them:
padding: @drawer-footer-padding-top 0 @drawer-footer-padding-bottom;|
|
||
| .@{drawer-prefix-cls}__confirm-btn { | ||
| order: 0; | ||
|
|
||
| &.reverse { | ||
| margin-left: var(--ti-drawer-divider-footer-button-margin-left); | ||
| margin-left: 8px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Variables for Button Spacing
The margin-left for the confirm and cancel buttons is set to 8px. Using a variable for this spacing would improve consistency and ease adjustments.
Define a variable:
@drawer-button-spacing: 8px;Then apply it:
margin-left: @drawer-button-spacing;Also applies to: 221-221
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/drawer/vars.less (1)
Line range hint
1-12: Typographical error in the header commentIn the header comment, the year is incorrectly listed as "202" instead of "2020".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/base/vars.less (1 hunks)
- packages/theme/src/drawer/vars.less (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/base/vars.less
🧰 Additional context used
🔇 Additional comments (1)
packages/theme/src/drawer/vars.less (1)
13-41: Variables updated to align with new design systemThe refactoring correctly updates the variable names to the new
--tv-prefix and references the appropriate design tokens.
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
Style Updates
Refactor
--ti-to--tv-prefixed CSS variables for consistency across the drawer component.