refactor(transfer): [transfer] update css vars #2320
Conversation
WalkthroughThe pull request involves a comprehensive update to the CSS variables used in the transfer component's stylesheets. The primary change is the renaming of CSS variables from a 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: 4
🧹 Outside diff range and nitpick comments (3)
packages/theme/src/transfer/index.less (3)
35-35: Check the definition of--tv-Transfer-font-sizeEnsure that the CSS variable
--tv-Transfer-font-sizeis defined in your variables file and is correctly referenced here.
296-297: Reconsider settingline-heightto header heightUsing
line-height: var(--tv-Transfer-header-height);matches the line height to the header's total height. This may cause vertical alignment issues with the text. Consider using a line height based on the font size for better text rendering.
346-359: Ensure consistent dimensions for header sort elementsVariables like
--tv-Transfer-header-sort-widthare used for both width and height of sort elements. Verify that these dimensions are appropriate and consistent with the design guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/transfer/index.less (17 hunks)
- packages/theme/src/transfer/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
packages/theme/src/transfer/vars.less (2)
13-243: Consistent Renaming to New Naming ConventionThe CSS variables have been consistently renamed from
--ti-to--tv-, aligning with the updated naming convention. This improves code clarity and maintainability.
13-13: Verify the Method Name Change Alignment with Project ConventionsAt line 13, the method name has been changed from
.component-css-vars-transfer()to.inject-Transfer-vars(). Please ensure that this new method name conforms to the project's naming conventions and that all references to the old method name are updated throughout the codebase.Run the following script to check for any remaining occurrences of the old method name:
✅ Verification successful
Method Name Change Alignment Verified
All references to
.component-css-vars-transfer()have been successfully updated to.inject-Transfer-vars()throughout the codebase, conforming to project naming conventions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any occurrences of .component-css-vars-transfer in the codebase. # Test: Search for the old method name. Expect: No occurrences found. rg --type less '\.component-css-vars-transfer\('Length of output: 50
Script:
#!/bin/bash # Description: Perform a case-insensitive search for .component-css-vars-transfer in all LESS files. # Test: Ensure no occurrences of the old method name exist. rg --type less -i '\.component-css-vars-transfer\('Length of output: 53
Script:
#!/bin/bash # Description: Use ast-grep to search for any usage of .component-css-vars-transfer in all LESS files. # Test: Ensure no occurrences of the old method name exist. ast-grep --lang css --pattern $'\.component-css-vars-transfer($_)' '**/*.less'Length of output: 481
Script:
#!/bin/bash # Description: List all .less files in the codebase to verify their existence and paths. fd --extension lessLength of output: 29815
Script:
#!/bin/bash # Description: Perform a comprehensive search for .component-css-vars-transfer in all .less files. # Search for the old method name with word boundaries to ensure accurate matches. rg --type less '\.component-css-vars-transfer\('Length of output: 50
packages/theme/src/transfer/index.less (4)
30-30: Ensure the mixin.inject-Transfer-vars();is correctly definedThe mixin
.inject-Transfer-vars();is used here. Please verify that it is correctly defined and imported, replacing the previous mixin.component-css-vars-transfer();if applicable.
83-86: Verify definitions of padding variables for buttonsThe variables for button padding (
--tv-Transfer-button-padding-left, etc.) are used here. Please ensure they are correctly defined in your variables file.
127-130: Ensure disabled state variables are definedVariables like
--tv-Transfer-button-disabled-border-color,--tv-Transfer-button-disabled-bg-color, and--tv-Transfer-button-disabled-text-colorare used for styling the disabled state. Verify that these variables are defined and match the design specifications.
248-249: Confirm hover state variables are definedThe variables
--tv-Transfer-panel-item-hover-bg-colorand--tv-Transfer-panel-item-hover-text-colorare applied on hover. Please ensure they are defined and provide the desired visual effect.
| // 面板头部边框色 | ||
| --ti-transfer-header-border-color: rgba(0, 0, 0, 0.08); | ||
| --tv-Transfer-header-border-color: #00000014; // 没有对应common变量值 |
There was a problem hiding this comment.
Avoid Hardcoded Color Values Without Variable Reference
At line 109, the variable --tv-Transfer-header-border-color is assigned a hardcoded color value #00000014 with the comment // 没有对应common变量值 ("No corresponding common variable value").
To maintain consistency and ease future theming, consider defining a common variable for this color in the global variables file. This allows for centralized management of color values and enhances themability.
Apply this diff to define and use a common variable:
Define the new common variable in the global variables file:
+ // In packages/theme/src/common/variables.less
+ --tv-color-border-light: #00000014;Update the assignment in vars.less:
- --tv-Transfer-header-border-color: #00000014; // 没有对应common变量值
+ --tv-Transfer-header-border-color: var(--tv-color-border-light);Committable suggestion was skipped due to low confidence.
| margin-left: var(--tv-Transfer-panel-filter-padding-left); | ||
| margin-right: var(--tv-Transfer-panel-filter-padding-right); | ||
| margin-top: var(--tv-Transfer-panel-filter-padding-top); | ||
| margin-bottom: var(--tv-Transfer-panel-filter-padding-bottom); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate the change from padding to margin
The padding has been changed to margin to address spacing issues with absolute positioned elements. Ensure that this change does not introduce unintended layout shifts or affect the component's responsiveness.
|
|
||
| &:not(.is-disabled) { | ||
| border-color: var(--ti-transfer-button-bg-color) !important; | ||
| border-color: var(--tv-Transfer-button-bg-color) !important; |
There was a problem hiding this comment.
Potential mismatch between border-color and background-color variables
Setting border-color to var(--tv-Transfer-button-bg-color) might not reflect the intended design. Typically, a separate variable for border-color is used.
Consider introducing a specific border color variable:
- border-color: var(--tv-Transfer-button-bg-color) !important;
+ border-color: var(--tv-Transfer-button-border-color) !important;Ensure that --tv-Transfer-button-border-color is defined appropriately.
Committable suggestion was skipped due to low confidence.
| margin-left: var(--tv-Transfer-buttons-defaultButton-margin-left); | ||
| } | ||
|
|
||
| & + .@{button-prefix-cls} { | ||
| margin-right: var(--ti-transfer-buttons-defaultButton-margin-right); | ||
| margin-top: var(--ti-transfer-buttons-defaultButton-margin-top); | ||
| margin-bottom: var(--ti-transfer-buttons-defaultButton-margin-bottom); | ||
| margin-right: var(--tv-Transfer-buttons-defaultButton-margin-right); | ||
| margin-top: var(--tv-Transfer-buttons-defaultButton-margin-top); | ||
| margin-bottom: var(--tv-Transfer-buttons-defaultButton-margin-bottom); |
There was a problem hiding this comment.
Inconsistent variable naming: buttons vs. button
There is a mix of variable names using both --tv-Transfer-buttons- and --tv-Transfer-button- prefixes. Please ensure consistency in variable naming to avoid confusion and potential errors.
Consider updating the variable names for consistency:
- margin-left: var(--tv-Transfer-buttons-defaultButton-margin-left);
+ margin-left: var(--tv-Transfer-button-defaultButton-margin-left);Repeat this change for all occurrences to maintain a consistent naming convention.
Committable suggestion was skipped due to low confidence.
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