refactor(guide): [guide] refactor guide theme vars #2207
Conversation
WalkthroughThe pull request introduces significant changes to the CSS variable naming conventions within the theme's guide component. It replaces the prefix 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: 3
🧹 Outside diff range and nitpick comments (4)
packages/theme/src/guide/index.less (2)
35-47: LGTM. Consider using variables for padding values.The changes in the
.shepherd-headerclass are well-implemented:
- CSS variable names have been consistently updated from
--ti-to--tv-.- Explicit padding values have been set, which improves consistency.
Consider using CSS variables for padding values (line 40) to maintain flexibility and consistency with the overall theming approach. For example:
- padding: 24px 24px 0; + padding: var(--tv-Guide-header-padding-top) var(--tv-Guide-header-padding-horizontal) 0;This would allow for easier theme customization in the future.
287-287: LGTM. Consistent variable renaming, but note duplication.The change in the
.@{guide-prefix-cls}__z-topclass is correct:
- The background color variable has been updated from
--ti-to--tv-, maintaining consistency with the overall variable renaming strategy.This change appears to be a duplicate of the one made on line 185. Consider reviewing if both instances are necessary or if one can be removed to avoid redundancy.
packages/theme/src/guide/vars.less (2)
29-29: Rename variable to remove redundancy in 'text-text-color'.The variable
--tv-Guide-text-text-colorcontains a redundant 'text-text' in its name. Renaming it to--tv-Guide-text-colorwould improve clarity and maintain consistency with naming conventions.Apply this diff to rename the variable:
- --tv-Guide-text-text-color: var(--tv-color-text-secondary); + --tv-Guide-text-color: var(--tv-color-text-secondary);
45-45: Clarify variable name by specifying 'border-color'.The variable
--tv-Guide-button-bordermay be ambiguous, as it doesn't specify the border property it represents. Renaming it to--tv-Guide-button-border-colorenhances clarity and aligns with other variables like--tv-Guide-button-hover-border-color.Apply this diff to rename the variable:
- --tv-Guide-button-border: var(--tv-color-border-secondary); + --tv-Guide-button-border-color: var(--tv-color-border-secondary);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/guide/index.less (7 hunks)
- packages/theme/src/guide/vars.less (1 hunks)
🔇 Additional comments (13)
packages/theme/src/guide/index.less (12)
6-6: LGTM. Verify the new injection method.The change from
.component-css-vars-guide()to.inject-Guide-vars()looks good. The new method name is more specific and follows a clearer naming convention.Please ensure that the
.inject-Guide-vars()method is properly defined and working as expected in the relevant files.
9-19: LGTM. Consistent variable renaming and improved flexibility.The changes in the
.shepherd-arrowclass are well-implemented:
- CSS variable names have been consistently updated from
--ti-to--tv-.- The use of
calc()for arrow dimensions (heightandwidth) allows for more flexible sizing.These changes improve consistency and maintainability.
66-79: LGTM. Improved cancel icon styling and customization.The changes in the
.shepherd-cancel-iconclass are well-implemented:
- Margin values have been updated to improve positioning.
- New styles for the SVG icon have been added, using CSS variables for dimensions and colors.
- Hover styles for the SVG icon have been updated, also using CSS variables.
These changes enhance the customization options for the cancel icon and maintain consistency with the overall theming approach.
85-106: LGTM. Improved text styling and scrollbar customization.The changes in the
.shepherd-textclass are well-implemented:
- CSS variable names have been consistently updated from
--ti-to--tv-.- Padding has been adjusted to improve the layout (line 88).
- Scrollbar styles have been refined, including width, height, and border color customization.
These changes enhance the appearance and usability of the text area while maintaining consistency with the overall theming approach.
Line range hint
120-136: LGTM. Improved footer styling and layout.The changes in the
.shepherd-footerclass are well-implemented:
- CSS variable names have been consistently updated from
--ti-to--tv-.- Border radius variables have been applied to improve customization options.
- Padding and positioning values have been adjusted to enhance the layout.
These changes improve the footer's appearance and maintain consistency with the overall theming approach.
141-159: LGTM. Enhanced button customization options.The changes in the
.shepherd-buttonclass are well-implemented:
- CSS variable names have been consistently updated from
--ti-to--tv-.- New variables for button dimensions, colors, and styles have been introduced, improving customization options.
- Hover and disabled states have been updated to use the new variables.
These changes significantly enhance the button's customization possibilities while maintaining consistency with the overall theming approach.
165-168: LGTM. Improved readability and layout adjustment.The changes in the
.only-contentclass are appropriate:
- An empty line has been added, improving code readability.
- Setting
padding-top: 0for the header in the.only-contentcase adjusts the layout when only content is displayed.These changes enhance code organization and provide better control over the component's appearance in different scenarios.
185-185: LGTM. Consistent variable renaming.The change in the
.@{guide-prefix-cls}__z-topclass is correct:
- The background color variable has been updated from
--ti-to--tv-, maintaining consistency with the overall variable renaming strategy.This change ensures that the z-top element's background color remains customizable through the new variable naming convention.
190-190: LGTM. Consistent variable renaming.The change in the
.shepherd-element.shepherd-has-title[data-popper-placement^='bottom']selector is correct:
- The background color variable has been updated from
--ti-to--tv-, maintaining consistency with the overall variable renaming strategy.This change ensures that the arrow's background color for bottom-placed elements with titles remains customizable through the new variable naming convention.
197-203: LGTM. Consistent variable renaming.The changes in the
.shepherd-elementclass are correct:
- The background color variable has been updated from
--ti-to--tv-.- The border-radius variable has been updated from
--ti-to--tv-.These changes maintain consistency with the overall variable renaming strategy, ensuring that the shepherd element's background color and border-radius remain customizable through the new variable naming convention.
228-240: LGTM. Consistent variable renaming for arrow positioning.The changes in the arrow positioning for different placements are correct:
- The arrow width variable has been updated from
--ti-to--tv-for all placements (top, bottom, left, right).- The use of
calc()for arrow positioning remains unchanged, maintaining the existing layout logic.These changes ensure consistency with the overall variable renaming strategy while preserving the correct arrow positioning for various element placements.
Line range hint
1-287: Overall LGTM. Comprehensive variable renaming and minor improvements.This refactoring of the guide theme variables is well-executed:
- CSS variables have been consistently renamed from
--ti-to--tv-throughout the file.- Minor adjustments to padding, layout, and styles have been made, improving the overall appearance and customization options.
- The changes maintain the existing functionality while enhancing theme consistency.
The refactoring improves code maintainability and aligns with what appears to be a broader effort to standardize variable naming conventions.
To ensure the changes haven't introduced any unintended side effects, please:
- Verify that all components using these variables have been updated accordingly.
- Run visual regression tests to confirm that the guide's appearance remains consistent with the previous version.
- Check if there's any documentation that needs to be updated to reflect these variable name changes.
packages/theme/src/guide/vars.less (1)
1-1: Ensure all references to the old mixin.component-css-vars-guide()are updated.The mixin name has been changed from
.component-css-vars-guide()to.inject-Guide-vars(). To prevent any issues, please verify that all instances of the old mixin are updated throughout the codebase.Run the following script to find any remaining references to the old mixin:
✅ Verification successful
All references to the old mixin
.component-css-vars-guide()have been successfully updated throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of the old mixin `.component-css-vars-guide()` # Test: Search for occurrences of the old mixin in LESS files. Expect: No matches. rg --type less '\.component-css-vars-guide\('Length of output: 47
Script:
#!/bin/bash # Description: Find usages of the old mixin `.component-css-vars-guide()` using ast-grep ast-grep --lang less --pattern " mixin_usage:Mixin(.component-css-vars-guide) { $$$ } "Length of output: 209
Script:
#!/bin/bash # Description: Find usages of the old mixin `.component-css-vars-guide()` using fd and grep fd --type f --extension less --exec grep -H '\.component-css-vars-guide\(' {} +Length of output: 17315
Script:
#!/bin/bash # Description: Find usages of the old mixin `.component-css-vars-guide()` using fd and grep -F fd --type f --extension less --exec grep -FH '.component-css-vars-guide(' {} +Length of output: 25212
Script:
#!/bin/bash # Description: Find usages of the old mixin `.component-css-vars-guide()` using fd and xargs with grep -F fd --type f --extension less | xargs grep -FH '.component-css-vars-guide('Length of output: 76
| // 分页提示文字颜色 | ||
| --ti-guide-progresss-text-color: #808080; | ||
| --tv-Guide-progresss-text-color: var(--tv-color-text-weaken); |
There was a problem hiding this comment.
Fix typo in variable name 'progresss' (extra 's').
The variable --tv-Guide-progresss-text-color has an extra 's' in 'progress'. Correcting the typo ensures that styles apply correctly without causing any errors.
Apply this diff to fix the typo:
- --tv-Guide-progresss-text-color: var(--tv-color-text-weaken);
+ --tv-Guide-progress-text-color: var(--tv-color-text-weaken);📝 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-Guide-progresss-text-color: var(--tv-color-text-weaken); | |
| --tv-Guide-progress-text-color: var(--tv-color-text-weaken); |
| // 引导框按钮宽度 | ||
| --ti-guide-button-width: calc(var(--ti-common-line-height-8, 60px) + 8px); | ||
| --tv-Guide-button-width: 68px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded button width with a variable for consistency.
The button width is set to 68px. Utilizing a size variable enhances maintainability and adherence to the design system.
Apply this diff to use a size variable:
- --tv-Guide-button-width: 68px;
+ --tv-Guide-button-width: var(--tv-size-width-sm);📝 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-Guide-button-width: 68px; | |
| --tv-Guide-button-width: var(--tv-size-width-sm); |
| --tv-Guide-arrow-height: 16px; | ||
| // 引导框箭头宽度 | ||
| --ti-guide-arrow-width: var(--ti-common-size-4x, 16px); | ||
| // 引导框头部左上边框圆角 | ||
| --ti-guide-header-border-top-left-radius: var(--ti-common-border-radius-4, 8px); | ||
| // 引导框头部右上边框圆角 | ||
| --ti-guide-header-border-top-right-radius: var(--ti-common-border-radius-4, 8px); | ||
| --tv-Guide-arrow-width: 16px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using existing size variables instead of hardcoded values for arrow dimensions.
The arrow height and width are set to 16px directly. To maintain consistency and ease future theming adjustments, consider using predefined variables like --tv-size-md.
Apply this diff to use size variables:
- --tv-Guide-arrow-height: 16px;
- --tv-Guide-arrow-width: 16px;
+ --tv-Guide-arrow-height: var(--tv-size-md);
+ --tv-Guide-arrow-width: var(--tv-size-md);📝 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-Guide-arrow-height: 16px; | |
| // 引导框箭头宽度 | |
| --ti-guide-arrow-width: var(--ti-common-size-4x, 16px); | |
| // 引导框头部左上边框圆角 | |
| --ti-guide-header-border-top-left-radius: var(--ti-common-border-radius-4, 8px); | |
| // 引导框头部右上边框圆角 | |
| --ti-guide-header-border-top-right-radius: var(--ti-common-border-radius-4, 8px); | |
| --tv-Guide-arrow-width: 16px; | |
| --tv-Guide-arrow-height: var(--tv-size-md); | |
| // 引导框箭头宽度 | |
| --tv-Guide-arrow-width: var(--tv-size-md); |
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