refactor(theme): [loading] refactor loading theme vars #2277
Conversation
WalkthroughThe changes in this pull request involve a comprehensive update to the loading component's styling in the LESS files. Key modifications include a shift in variable naming conventions from Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (5)
packages/theme/src/loading/vars.less (5)
13-13: Approve function name change with a minor suggestion.The new function name
.inject-Loading-vars()is more descriptive and aligns with its purpose. However, for consistency with typical CSS naming conventions, consider using kebab-case for the entire function name.Consider renaming the function to
.inject-loading-vars()for better alignment with CSS naming conventions.
15-15: Approve variable change with a suggestion for fallback.The variable renaming and value update improve consistency with the new naming scheme and color management. However, consider adding a fallback color to maintain backwards compatibility.
Consider adding a fallback color:
- --tv-Loading-icon-color: var(--tv-color-icon-active); + --tv-Loading-icon-color: var(--tv-color-icon-active, #191919);
17-17: Approve text color change with a suggestion for fallback.The variable renaming and value update are consistent with the new naming scheme. However, consider adding a fallback color to maintain backwards compatibility.
Consider adding a fallback color:
- --tv-Loading-text-color: var(--tv-color-text-secondary); + --tv-Loading-text-color: var(--tv-color-text-secondary, #4e5969);
19-19: Approve background color change with a suggestion for fallback.The variable renaming and value update improve consistency with the new naming scheme and color management. However, consider adding a fallback color to maintain backwards compatibility.
Consider adding a fallback color:
- --tv-Loading-mask-bg-color: var(--tv-color-bg); + --tv-Loading-mask-bg-color: var(--tv-color-bg, #fff);
Remaining
--ti-Prefixed Variables FoundSeveral loading-related CSS variables with the
--ti-prefix were found in the following files:
packages/theme/src/upload-list/vars.lesspackages/theme/src/upload-list/index.lesspackages/theme/src/tree/vars.lesspackages/theme/src/tree/index.lesspackages/theme/src/modal/vars.lesspackages/theme/src/modal/index.lesspackages/theme/src/float-button/vars.lesspackages/theme/src/float-button/index.lesspackages/theme/src/base/basic-var.lesspackages/theme-mobile/src/pull-refresh/index.lesspackages/theme-mobile/src/pull-refresh/vars.lesspackages/theme-mobile/src/loading/vars.lesspackages/theme-mobile/src/loading/index.lesspackages/theme-mobile/src/button/vars.lesspackages/theme-mobile/src/button/index.lessPlease update these variables to use the
--tv-prefix to maintain consistency and align with the new naming conventions.🔗 Analysis chain
Line range hint
1-53: Overall assessment: Approve with recommendationsThe refactoring of the loading component's CSS variables significantly improves consistency, maintainability, and alignment with the design system. The changes from
--ti-to--tv-prefix and the use of semantic design system variables enhance the overall quality of the code.Recommendations:
- Verify all new variable values to ensure they match the intended designs.
- Consider adding fallback values for color and size variables to maintain backwards compatibility.
- Ensure that these changes are reflected in the component's documentation.
- Update any related test files to account for these new variable names.
To ensure consistency across the entire codebase, please run the following script to check for any remaining
--ti-prefixed variables related to loading:This will help identify any variables that might have been missed during the refactoring process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining --ti- prefixed loading variables rg --type less "--ti-.*loading"Length of output: 450
Script:
#!/bin/bash # Search for any remaining --ti- prefixed loading variables in .less files rg --type less -e '--ti-.*loading'Length of output: 6601
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/loading/index.less (3 hunks)
- packages/theme/src/loading/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (14)
packages/theme/src/loading/vars.less (4)
28-32: Approve fullscreen loading icon size changes with verification suggestion.The variable renaming and value updates improve consistency with the new naming scheme and leverage the design system's size variables. This change enhances maintainability and provides flexibility for different icon sizes.
Please verify that the new design system variables provide the intended sizes for each variant (small, medium, large). Ensure that these changes don't unintentionally alter the appearance of the loading icons.
To verify, you can run the following script:
#!/bin/bash # Search for the definitions of the used design system variables rg --type less "tv-icon-size|tv-size-height-xs|tv-size-height-md"Also applies to: 35-39
42-46: Approve default loading spinner height changes with verification suggestion.The variable renaming and value updates improve consistency with the new naming scheme and leverage the design system's size variables. This change enhances maintainability and provides flexibility for different spinner sizes.
Please verify that the new design system variables provide the intended heights for each variant (small, medium, large) of the default loading spinner. Ensure that these changes don't unintentionally alter the appearance of the loading spinners.
To verify, you can run the following script:
#!/bin/bash # Search for the definitions of the used design system variables rg --type less "tv-icon-size|tv-size-height-xs|tv-size-height-md"
49-53: Approve default loading spinner width changes with verification suggestion.The variable renaming and value updates improve consistency with the new naming scheme and leverage the design system's size variables. This change enhances maintainability and provides flexibility for different spinner sizes.
Please verify that the new design system variables provide the intended widths for each variant (small, medium, large) of the default loading spinner. Ensure that these changes don't unintentionally alter the appearance of the loading spinners.
To verify, you can run the following script:
#!/bin/bash # Search for the definitions of the used design system variables rg --type less "tv-icon-size|tv-size-height-xs|tv-size-height-md"
23-25: Approve margin changes with verification suggestion.The variable renaming and value updates improve consistency with the new naming scheme and leverage the design system's spacing variables. The change from explicit pixel values to design system variables enhances maintainability.
Please verify that
var(--tv-space-xl)provides the intended vertical margin. Also, confirm that setting the horizontal margin to 0 is intentional.To verify, you can run the following script:
#!/bin/bash # Search for the definition of --tv-space-xl rg --type less "tv-space-xl"packages/theme/src/loading/index.less (10)
19-19: Mixin variables injected correctlyThe mixin
.inject-Loading-vars()is correctly used to inject the new loading component variables.
24-24: Updated loading mask background color variableThe
background-colorproperty now correctly usesvar(--tv-Loading-mask-bg-color).
40-41: Fullscreen small spinner dimensions updatedThe height and width of the small fullscreen spinner are correctly updated to use
var(--tv-Loading-fullscreen-small-height)andvar(--tv-Loading-fullscreen-small-width).
47-48: Fullscreen medium spinner dimensions updatedThe height and width of the medium fullscreen spinner are correctly updated to use
var(--tv-Loading-fullscreen-medium-height)andvar(--tv-Loading-fullscreen-medium-width).
54-55: Fullscreen large spinner dimensions updatedThe height and width of the large fullscreen spinner are correctly updated to use
var(--tv-Loading-fullscreen-large-height)andvar(--tv-Loading-fullscreen-large-width).
70-72: Loading text styles updatedThe color, margin, and font size of the loading text are correctly updated with the new variables:
color: var(--tv-Loading-text-color);margin: var(--tv-Loading-text-margin-y) var(--tv-Loading-text-margin-x);font-size: var(--tv-Loading-text-font-size);
78-79: Spinner small size dimensions updatedThe height and width of the small spinner are correctly updated to use
var(--tv-Loading-spinner-small-height)andvar(--tv-Loading-spinner-small-width).
86-87: Spinner medium size dimensions updatedThe height and width of the medium spinner are correctly updated to use
var(--tv-Loading-spinner-medium-height)andvar(--tv-Loading-spinner-medium-width).
94-95: Spinner large size dimensions updatedThe height and width of the large spinner are correctly updated to use
var(--tv-Loading-spinner-large-height)andvar(--tv-Loading-spinner-large-width).
100-100: SVG icon fill color updatedThe
fillproperty for the SVG icon now correctly usesvar(--tv-Loading-icon-color).
| --ti-loading-text-font-size: var(--ti-common-font-size-2, 16px); | ||
| // 加载图标自定义加载图标顶部外边距 | ||
| --ti-loading-spinner-margin-top: calc(var(--ti-common-space-0, 0px) - 21px); | ||
| --tv-Loading-text-font-size: var(--tv-font-size-lg); |
There was a problem hiding this comment.
💡 Codebase verification
Variable --tv-font-size-lg is not defined. Please define it or revert to using the previous variable with a fallback value.
🔗 Analysis chain
Approve font size change with a verification suggestion.
The variable renaming and value update improve consistency with the new naming scheme and use a more semantic font size variable. This change enhances maintainability.
Please verify that --tv-font-size-lg corresponds to 14px or the intended size. If not, consider adding a fallback size:
- --tv-Loading-text-font-size: var(--tv-font-size-lg);
+ --tv-Loading-text-font-size: var(--tv-font-size-lg, 14px);To verify, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of --tv-font-size-lg
rg --type less "tv-font-size-lg"
Length of output: 2558
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