feat: add space var to adapt to old theme #2581
Conversation
WalkthroughThe pull request introduces several modifications to CSS variable definitions across various components in the theme package. Key changes include replacing calculated width values with fixed pixel values for the anchor component, adding new spacing variables for layout flexibility, and updating padding and margin properties to utilize CSS variables instead of hardcoded values in components like the calendar view, dialog box, and drawer. These updates aim to enhance maintainability and responsiveness in styling. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![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 (23)
packages/theme/src/image-viewer/vars.less (1)
20-20: Document the intentional move away from responsive designThe change from calculated values (
calc()) to fixed pixel values reduces responsiveness but aligns with the old theme compatibility requirement. Consider adding a comment explaining this intentional design decision to prevent future confusion.Add a comment above these variables:
+ /* Fixed values to maintain compatibility with old theme */ --tv-ImageViewer-next-font-size: 24px; ... --tv-ImageViewer-prev-left: 32px; --tv-ImageViewer-next-right: 32px;Also applies to: 25-26
packages/theme/src/drawer/vars.less (1)
Line range hint
3-3: Fix incomplete copyright yearThe copyright notice has an incomplete year "202" which should be "2023".
Apply this fix:
-* Copyright (c) 202 - present Huawei Cloud Computing Technologies Co., Ltd. +* Copyright (c) 2023 - present Huawei Cloud Computing Technologies Co., Ltd.packages/theme/src/dialog-box/vars.less (1)
20-21: Consider enhancing the documentationThe variable addition follows the proper naming convention and is well-placed within the mixin. Consider adding an English translation to the comment for better international developer experience.
- // 弹窗容器与内容间距 + // 弹窗容器与内容间距 (Dialog container and content spacing) --tv-DialogBox-padding: var(--tv-space-xxxl);packages/theme/src/anchor/vars.less (1)
Line range hint
1-48: Document the rationale for fixed widthWhile the change adapts to an old theme, it would be helpful to:
- Add a comment explaining why a fixed width is necessary
- Document any known limitations or side effects
- Consider marking this as a temporary solution if there are plans to restore calculated values
Add a comment above the width declaration:
// 组件宽度 + // Fixed width to maintain compatibility with legacy theme --tv-Anchor-width: 160px;packages/theme/src/form-item/vars.less (2)
45-55: Document spacing scale variation across sizesThe margin-bottom spacing uses:
--tv-space-xxlfor lg, sm sizes--tv-space-xlfor xs sizeThis variation should be documented to explain the intentional difference in spacing for the xs size variant.
Consider adding a comment explaining why xs size uses a different spacing scale than other sizes.
33-55: Document theme migration requirementsThese spacing changes represent a significant shift from fixed pixels to CSS variables. To ensure a smooth transition:
- Update the theme documentation to reflect these new spacing variables
- Provide migration steps for projects using the old theme
- Consider adding a code comment block at the top of this file explaining the spacing scale used
Would you like me to help draft the documentation updates?
packages/theme/src/modal/vars.less (2)
43-43: Consider standardizing all header padding valuesWhile replacing fixed pixel values with CSS variables improves maintainability, the header's bottom padding (24px) remains as a fixed value. Consider standardizing all padding values to use the spacing system variables for better consistency.
--tv-Modal-box-header-padding-x: var(--tv-space-xxxl); --tv-Modal-box-header-padding-top: var(--tv-space-xxxl); - --tv-Modal-box-header-padding-bottom: 24px; + --tv-Modal-box-header-padding-bottom: var(--tv-space-xl);Also applies to: 45-45
85-85: Consider standardizing all footer padding valuesSimilar to the header section, consider using spacing system variables consistently for all padding values in the footer.
--tv-Modal-box-footer-padding-x: var(--tv-space-xxxl); - --tv-Modal-box-footer-padding-top: 24px; + --tv-Modal-box-footer-padding-top: var(--tv-space-xl); --tv-Modal-box-footer-padding-bottom: var(--tv-space-xxxl);Also applies to: 89-89
packages/theme/src/calendar-view/vars.less (1)
16-17: Consider adding migration guide.Since this change affects layout spacing, consider documenting the migration path for users of the old theme.
Would you like me to help draft migration documentation for this change?
packages/theme/src/dialog-box/index.less (4)
62-62: Consider using CSS variables for all spacing values.While using
--tv-DialogBox-paddingimproves maintainability, consider extracting the fixed24pxbottom padding to a CSS variable for consistent theming control.- padding: var(--tv-DialogBox-padding) var(--tv-DialogBox-padding) 24px; + padding: var(--tv-DialogBox-padding) var(--tv-DialogBox-padding) var(--tv-DialogBox-header-bottom-padding);
138-138: Maintain consistency in padding value types.For consistency with the header's padding approach, consider extracting the fixed
24pxtop padding to a CSS variable.- padding: 24px var(--tv-DialogBox-padding) var(--tv-DialogBox-padding); + padding: var(--tv-DialogBox-footer-top-padding) var(--tv-DialogBox-padding) var(--tv-DialogBox-padding);
170-170: Unify spacing variables across drawer variant.The drawer variant uses the same mixed approach of fixed and variable values. Consider creating a unified set of spacing variables for the drawer variant to improve maintainability.
- padding: var(--tv-DialogBox-padding) 0 24px; + padding: var(--tv-DialogBox-padding) 0 var(--tv-DialogBox-drawer-header-bottom-padding); - padding: 0 var(--tv-DialogBox-padding); + padding: 0 var(--tv-DialogBox-padding); - padding: 24px 0 var(--tv-DialogBox-padding); + padding: var(--tv-DialogBox-drawer-footer-top-padding) 0 var(--tv-DialogBox-padding);Also applies to: 183-183, 187-187
Line range hint
62-187: Consider documenting spacing variables in theme documentation.The introduction of
--tv-DialogBox-paddingimproves theme customization. To ensure proper usage:
- Document the new spacing variable in the theme documentation
- Consider providing default values that match the old theme for backward compatibility
- Add examples of customizing these variables for different use cases
packages/theme/src/tabs/vars.less (4)
25-25: Consider using CSS variables for button dimensionsUsing hardcoded pixel values reduces maintainability and flexibility. Since both dimensions are identical (18px), consider:
- Using a single CSS variable for both dimensions
- Deriving the value from the design system's spacing scale
- --tv-Tabs-new-height: 18px; - --tv-Tabs-new-width: 18px; + --tv-Tabs-new-size: var(--tv-size-square-sm, 18px); + --tv-Tabs-new-height: var(--tv-Tabs-new-size); + --tv-Tabs-new-width: var(--tv-Tabs-new-size);Also applies to: 27-27
63-63: Use spacing variable for consistencyThe hardcoded 24px value should use the design system's spacing scale for consistency with other spacing variables in the file.
- --tv-Tabs-small-item-margin-right: 24px; + --tv-Tabs-small-item-margin-right: var(--tv-space-xxl, 24px);
139-139: Consolidate duplicate height valuesBoth height variables are set to 52px. Consider:
- Using a single variable for both heights
- Aligning with standard component heights
+ --tv-Tabs-more-height: var(--tv-size-height-xxl, 52px); - --tv-Tabs-more-icon-height: 52px; + --tv-Tabs-more-icon-height: var(--tv-Tabs-more-height); - --tv-Tabs-more-left-box-height: 52px; + --tv-Tabs-more-left-box-height: var(--tv-Tabs-more-height);Also applies to: 143-143
Line range hint
25-147: Consider impact on design system consistencyThe introduction of multiple hardcoded pixel values might:
- Reduce maintainability when theme updates are needed
- Break responsive design patterns
- Create inconsistencies with the design system's scale
Consider creating a comprehensive spacing/sizing scale in the design system and deriving all values from it.
packages/theme/src/drawer/index.less (2)
158-158: Consider using CSS variables for all padding valuesWhile the horizontal padding now uses a CSS variable, the bottom padding (24px) is still hardcoded. For better maintainability and consistency, consider using CSS variables for all padding values.
- padding: var(--tv-Drawer-padding) 0 24px; + padding: var(--tv-Drawer-padding) 0 var(--tv-Drawer-header-bottom-padding, 24px);
209-209: Consider standardizing vertical paddingThe footer's top padding (24px) is hardcoded while the bottom uses a variable. Additionally, this creates vertical asymmetry with the header section. Consider:
- Using CSS variables for all padding values
- Standardizing the vertical spacing between header, body, and footer
- padding: 24px 0 var(--tv-Drawer-padding); + padding: var(--tv-Drawer-section-spacing, 24px) 0 var(--tv-Drawer-padding);packages/theme/src/steps/vars.less (2)
82-82: Consider maintaining calculated values for better flexibility.While using
24pxdirectly reduces calculation overhead, consider keeping the calculationcalc(var(--tv-space-base, 4px) * 6)to maintain flexibility when base spacing needs to be adjusted globally.- --tv-Steps-icon-small-size: 24px; + --tv-Steps-icon-small-size: calc(var(--tv-space-base, 4px) * 6);
252-252: Consider maintaining calculated values for better flexibility.Similar to the earlier comment, while using
8pxdirectly reduces calculation overhead, consider keeping the calculationcalc(var(--tv-size-base, 4px) * 2)to maintain flexibility when base sizing needs to be adjusted globally.- --tv-Steps-timeline-dot-width: 8px; + --tv-Steps-timeline-dot-width: calc(var(--tv-size-base, 4px) * 2);packages/theme/src/calendar-view/index.less (1)
Line range hint
1-589: Consider centralizing color and opacity valuesThe file contains numerous repeated color definitions using rgba values. Consider refactoring to improve maintainability:
- Extract common colors into CSS variables:
+ :root { + --tv-blue-base: #1890ff; + --tv-green-base: #00a874; + --tv-red-base: #eb171f; + --tv-yellow-base: #f36900; + --tv-purple-base: #745ef7; + --tv-cyan-base: #00a2b5; + --tv-grey-base: #8d959e; + --tv-opacity-light: 0.1; + }
- Replace rgba values with CSS color-mix:
- background-color: rgba(0, 103, 209, 0.1); + background-color: color-mix(in srgb, var(--tv-blue-base) var(--tv-opacity-light), transparent);This would:
- Reduce code duplication
- Make color scheme updates easier
- Maintain consistency across themes
- Improve maintainability
packages/theme/src/base/vars.less (1)
415-416: LGTM! The new spacing variables follow the established patterns.The additions:
- Follow the existing naming convention
- Use consistent calculation method
- Include clear usage documentation in comments
Consider documenting the migration path for users of the old theme, explaining:
- Which old theme variables map to these new spacing variables
- Any breaking changes in spacing that might affect layouts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
packages/theme/src/anchor/vars.less(1 hunks)packages/theme/src/base/old-theme.less(1 hunks)packages/theme/src/base/vars.less(1 hunks)packages/theme/src/calendar-view/index.less(2 hunks)packages/theme/src/calendar-view/vars.less(1 hunks)packages/theme/src/dialog-box/index.less(5 hunks)packages/theme/src/dialog-box/vars.less(1 hunks)packages/theme/src/drawer/index.less(4 hunks)packages/theme/src/drawer/vars.less(1 hunks)packages/theme/src/form-item/index.less(1 hunks)packages/theme/src/form-item/vars.less(1 hunks)packages/theme/src/image-viewer/vars.less(1 hunks)packages/theme/src/link-menu/index.less(0 hunks)packages/theme/src/modal/vars.less(3 hunks)packages/theme/src/steps/vars.less(2 hunks)packages/theme/src/tabs/vars.less(4 hunks)packages/theme/src/wizard/vars.less(1 hunks)
💤 Files with no reviewable changes (1)
- packages/theme/src/link-menu/index.less
🔇 Additional comments (19)
packages/theme/src/image-viewer/vars.less (1)
20-20: Verify these fixed values match the old theme exactly
The changes replace calculated values with fixed pixel values:
--tv-ImageViewer-next-font-size: from calculated to24px--tv-ImageViewer-prev-left: from calculated to32px--tv-ImageViewer-next-right: from calculated to32px
Also applies to: 25-26
packages/theme/src/drawer/vars.less (1)
20-21: LGTM! Verify the space variable dependency
The new padding variable follows the established naming convention and includes appropriate documentation. However, let's verify that the --tv-space-xxxl variable it depends on exists in the theme system.
✅ Verification successful
The --tv-space-xxxl variable is properly defined and consistently used
The variable is defined in the base theme files (base/vars.less and base/old-theme.less) as calc(var(--tv-space-base) * 8) with a clear comment indicating its purpose for container-content spacing. It's consistently used across multiple components including Modal, DialogBox, and CalendarView for similar padding purposes, making this usage in Drawer perfectly aligned with the design system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and usage of --tv-space-xxxl variable
# Search for the definition of --tv-space-xxxl
echo "Searching for --tv-space-xxxl definition:"
rg --type less "tv-space-xxxl" packages/theme/src/
# Check if this variable is used consistently in other components
echo -e "\nChecking usage in other components:"
rg --type less "var\(--tv-space-xxxl\)" packages/theme/src/
Length of output: 1951
packages/theme/src/dialog-box/vars.less (1)
21-21: Verify the dependency on --tv-space-xxxl variable
The implementation looks correct, but let's ensure the --tv-space-xxxl variable is properly defined in the theme system.
✅ Verification successful
The --tv-space-xxxl variable is properly defined in the theme system
The variable is defined in packages/theme/src/base/vars.less and packages/theme/src/base/old-theme.less as calc(var(--tv-space-base) * 8). It's also consistently used across multiple components including modal, drawer, calendar-view, and dialog-box for padding purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if --tv-space-xxxl is defined in the theme system
# Expected: Find definition of --tv-space-xxxl in theme files
# Search for space variable definition
rg --type=less "tv-space-xxxl" packages/theme/src/
Length of output: 981
packages/theme/src/form-item/vars.less (2)
33-35: Verify space variable compatibility across themes
The introduction of --tv-space-xxl variables needs verification to ensure compatibility with both new and old themes. Additionally, the new --tv-FormItem-inline-margin-right variable might affect form layouts.
Consider documenting these spacing changes in the theme migration guide if this introduces layout changes.
✅ Verification successful
Space variable is properly defined and consistently used
The --tv-space-xxl variable is well-defined in both the new theme (base/vars.less) and old theme (base/old-theme.less), with slightly different multipliers (6x and 5x of --tv-space-base respectively). The variable is also consistently used across form-item and steps components, indicating it's a standard spacing token.
- Defined in
packages/theme/src/base/vars.lessas--tv-space-xxl: calc(var(--tv-space-base) * 6) - Defined in
packages/theme/src/base/old-theme.lessas--tv-space-xxl: calc(var(--tv-space-base) * 5)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the space variable is defined in theme files
rg --type=less "tv-space-xxl" packages/theme/
Length of output: 814
40-40: Consider layout impact of label padding change
The change from a fixed padding to var(--tv-space-xxl) for label padding could affect form layouts, especially in dense forms or those with long labels.
✅ Verification successful
Let me search for any usage of this variable to better understand the impact.
Based on the search results, I can now generate the final response:
Label padding change appears safe to implement
The variable --tv-FormItem-label-padding-right is only used within the core theme files and has no custom overrides in the codebase. The change to use var(--tv-space-xxl) aligns with the design system's spacing scale and maintains consistency.
- The padding is applied in
packages/theme/src/form-item/index.lessfor the standard form item label - No custom implementations or overrides were found that could be affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for custom overrides of the old padding value that might need updating
rg --type=less "FormItem-label-padding-right:" packages/
Length of output: 153
Script:
#!/bin/bash
# Search for usage of the FormItem-label-padding-right variable
rg --type=less "FormItem-label-padding-right" packages/ -B2 -A2
# Search for any custom form item styles that might be affected
rg --type=less "\.tv-FormItem" packages/ -B2 -A2
Length of output: 852
packages/theme/src/modal/vars.less (2)
76-76: LGTM!
The change to use var(--tv-space-xxxl) for horizontal padding aligns with the spacing system while maintaining the intended zero vertical padding.
85-89: Verify the impact of removing footer button min-width
The removal of --tv-Modal-box-footerbtn-min-width could affect the layout of buttons in the modal footer. Please ensure this change doesn't cause unintended button sizing issues.
✅ Verification successful
No impact found from removing footer button min-width variable
The variable --tv-Modal-box-footerbtn-min-width appears to be unused in the codebase. No references to this variable were found in any Less files or other components, and no alternative button min-width styling was detected in modal footer contexts. The removal of this unused variable should not cause any layout issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of the removed variable
rg --type=less --type=vue "tv-Modal-box-footerbtn-min-width"
# Search for any button styling in modal footer that might need adjustment
ast-grep --pattern 'style="min-width: $$$"' --lang=vue
Length of output: 252
Script:
#!/bin/bash
# Search for any direct usage of the removed variable in less files
rg --type=less "tv-Modal-box-footerbtn-min-width"
# Search for any button styling in modal footer
rg --type=less "footerbtn.*min-width|footer.*button.*min-width"
# Look for modal footer button styles in any file
rg "Modal.*footer.*button|Modal.*footerbtn"
Length of output: 161
packages/theme/src/calendar-view/vars.less (2)
16-17: LGTM! Well-documented variable addition.
The new padding variable is properly documented and follows the existing naming conventions. The use of --tv-space-xxxl aligns with the design system's spacing scale.
16-17: Verify impact on old theme compatibility.
Since this PR aims to adapt to the old theme, we should verify if this spacing value matches the previous implementation.
✅ Verification successful
The spacing value aligns with the old theme implementation
The verification shows that --tv-space-xxxl is consistently defined in both the old theme (base/old-theme.less) and new theme (base/vars.less) as calc(var(--tv-space-base) * 8) with the same comment "容器与容器内容的间距" (container and content spacing). This confirms that using --tv-CalendarView-padding: var(--tv-space-xxxl) maintains compatibility with the old theme's spacing system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing padding definitions in old theme files
rg -g '*.less' -g '*.css' "CalendarView.*padding"
# Look for any existing usage of --tv-space-xxxl to understand its context
rg -g '*.less' -g '*.css' "tv-space-xxxl"
Length of output: 2814
packages/theme/src/dialog-box/index.less (2)
119-119: LGTM!
The horizontal padding using --tv-DialogBox-padding with zero vertical padding provides good flexibility for content layout.
165-166: LGTM!
The use of --tv-DialogBox-padding for horizontal margins provides consistent spacing in the drawer variant.
packages/theme/src/tabs/vars.less (1)
147-147: LGTM! Good optimization
Removing the unit from zero values is a CSS best practice.
packages/theme/src/drawer/index.less (3)
123-124: LGTM! Good use of CSS variables for consistent spacing
The change improves maintainability by using CSS variables instead of hardcoded values, ensuring consistent spacing across the component.
200-200: LGTM! Consistent use of spacing variable
Good use of the same padding variable to maintain consistent horizontal spacing throughout the drawer component.
Line range hint 1-2: Verify CSS variable definition and cross-component usage
The changes introduce usage of --tv-Drawer-padding variable, but we should verify:
- The variable's definition in the imported files
- Consistent usage across other components using similar spacing patterns
✅ Verification successful
CSS variable is properly defined and follows consistent spacing patterns
The verification shows that:
- The
--tv-Drawer-paddingvariable is properly defined invars.lessand inherits from--tv-space-xxxl - The variable is used consistently within the drawer component for various spacing needs
- The pattern of vertical padding with fixed horizontal spacing is common across many components, suggesting this is an intentional design pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for the variable definition
echo "Checking for variable definition:"
rg --type less "tv-Drawer-padding" packages/theme/src/
# Check for similar padding patterns in other components
echo -e "\nChecking for similar padding patterns in other components:"
rg --type less "padding: \d+px 0" packages/theme/src/
Length of output: 3159
packages/theme/src/form-item/index.less (1)
276-276: LGTM! Verify the CSS variable definition.
The change from a hardcoded value to var(--tv-FormItem-inline-margin-right) follows best practices for theme customization and aligns with the PR's objective of theme adaptation.
Let's verify the variable definition is properly set:
✅ Verification successful
CSS variable is properly defined and injected ✅
The variable --tv-FormItem-inline-margin-right is well-defined in vars.less and defaults to --tv-space-xxl. The variable is properly injected through the inject-FormItem-vars mixin which is called in index.less.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the CSS variable is defined in the theme system
# Look for the variable definition in theme files
# Search for the variable definition
rg --type less "tv-FormItem-inline-margin-right"
# Check if the variable is injected through the inject-FormItem-vars mixin
rg --type less "inject-FormItem-vars"
Length of output: 482
packages/theme/src/steps/vars.less (1)
250-250: LGTM! Good use of semantic spacing variable.
The change from a calculation to using the semantic spacing variable --tv-space-xxl improves maintainability and consistency with the design system.
packages/theme/src/calendar-view/index.less (1)
23-23: Verify CSS variable definitions and update documentation
The change to use --tv-CalendarView-padding variable improves theme customization capabilities and maintainability. However, we need to ensure proper implementation.
Let's verify the variable definitions and usage:
Consider adding a comment in the code or updating the component's documentation to explain:
- The purpose of this spacing variable
- Its relationship with the old theme mentioned in the PR title
- Any breaking changes for existing theme customizations
Also applies to: 33-33
✅ Verification successful
CSS variable is properly defined and used consistently
The variable --tv-CalendarView-padding is correctly defined in packages/theme/src/calendar-view/vars.less using the design system spacing token --tv-space-xxxl. The usage is consistent within the calendar view component and doesn't affect other components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the CSS variable is properly defined and documented
# Check for variable definition in vars.less
echo "Checking variable definition in vars.less:"
rg --type less "tv-CalendarView-padding" "packages/theme/src"
# Check if other components might be affected
echo "Checking usage in other components:"
rg --type less "tv-CalendarView-padding" "packages/theme"
# Check if documentation exists
echo "Checking documentation:"
fd -e md -e mdx . | xargs rg "tv-CalendarView-padding"
Length of output: 955
packages/theme/src/base/old-theme.less (1)
414-415: LGTM! Well-documented spacing variables.
The new spacing variables follow the existing scale pattern and are well documented with their intended usage:
--tv-space-xxl: For spacing between content blocks--tv-space-xxxl: For spacing between container and content
The multipliers (5x and 8x) provide a logical progression in the spacing scale.
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| .inject-anchor-vars() { | |||
| // 组件宽度 | |||
| --tv-Anchor-width: calc(var(--tv-size-base, 4px) * 40); | |||
| --tv-Anchor-width: 160px; | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider maintaining calculated width for consistency
The codebase shows consistent usage of calculated widths based on --tv-size-base across multiple components:
- Steps component uses it for heights and icon sizes
- Milestone component for node size and flag dimensions
- Card component for different width variants (large, medium, small, mini)
Replacing the calculated width with a fixed value in the Anchor component deviates from this pattern and could impact:
- Design system consistency
- Component scalability with the base unit
- Maintainability when base unit changes
Consider reverting to calc(var(--tv-size-base, 4px) * 40) to maintain consistency with the design system.
🔗 Analysis chain
Consider maintaining flexibility with calculated values
Replacing calc(var(--tv-size-base, 4px) * 40) with a fixed 160px reduces the component's flexibility. While this might help compatibility with an old theme, it could impact:
- Responsiveness across different screen sizes
- Accessibility for users with custom zoom levels
- Consistency with the design system's base unit scaling
Let's check if other components still use calculated widths:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other calculated widths using tv-size-base
rg "calc\(.*--tv-size-base.*\)" packages/theme/src/
Length of output: 18147
| @@ -60,7 +60,7 @@ | |||
| // 节点标题上外边距 | |||
| --tv-Wizard-node-title-margin-top: var(--tv-space-md, 8px); | |||
| // 页向导模式按钮组上外边距 | |||
| --tv-Wizard-button-group-margin-top: calc(var(--tv-space-base, 4px) * 9); | |||
| --tv-Wizard-button-group-margin-top: 36px; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider keeping the calculated value for better theme adaptability
The change from calc(var(--tv-space-base, 4px) * 9) to 36px makes the margin less adaptable to theme customization. While both values are numerically equivalent (4px * 9 = 36px), the fixed pixel value won't automatically adjust when --tv-space-base is customized.
This change also introduces inconsistency, as other spacing variables in the file continue to use CSS variables (e.g., --tv-space-md, --tv-space-lg).
Consider reverting to the calculated value:
--- --tv-Wizard-button-group-margin-top: 36px;
+++ --tv-Wizard-button-group-margin-top: calc(var(--tv-space-base, 4px) * 9);📝 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-Wizard-button-group-margin-top: 36px; | |
| --tv-Wizard-button-group-margin-top: calc(var(--tv-space-base, 4px) * 9); |
| @@ -84,15 +84,15 @@ | |||
| // bordercard类型非选中项文本色 | |||
| --tv-Tabs-dark-text-color: var(--tv-color-text-disabled, #c2c2c2); | |||
| // bordercard类型标签项上外边距 | |||
| --tv-Tabs-dark-margin-top: calc(var(--tv-space-base, 4px) + 1px); | |||
| --tv-Tabs-dark-margin-top: 5px; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review unusual spacing values
The current values raise some concerns:
5pxmargin is non-standard and should align with the spacing scale47pxheight is an odd number that might cause sub-pixel rendering issues
- --tv-Tabs-dark-margin-top: 5px;
+ --tv-Tabs-dark-margin-top: var(--tv-space-base, 4px);
- --tv-Tabs-dark-item-height: 47px;
+ --tv-Tabs-dark-item-height: var(--tv-size-height-xl, 48px);Also applies to: 95-95
| --tv-space-xxl: calc(var(--tv-space-base) * 5); // 内容块之间的间距 | ||
| --tv-space-xxxl: calc(var(--tv-space-base) * 8); // 容器与容器内容的间距 |
There was a problem hiding this comment.
💡 Codebase verification
Consider replacing hardcoded spacing values with new CSS variables
The verification reveals:
- The new spacing variables (
--tv-space-xxland--tv-space-xxxl) are already being used consistently in several components like Modal, Drawer, Form, and Calendar. - However, there are numerous instances of hardcoded values that match these new spacing variables:
- Multiple occurrences of
20px(equivalent to--tv-space-xxl) in components like Input, Grid, Popover - Several instances of
32px(equivalent to--tv-space-xxxl) in components like Button, Card, Nav Menu
- Multiple occurrences of
To maintain consistency, consider:
- Replace
20pxwithvar(--tv-space-xxl)where appropriate - Replace
32pxwithvar(--tv-space-xxxl)where appropriate
🔗 Analysis chain
Verify consistent usage of new spacing variables.
Let's ensure these new spacing variables are being used consistently across components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing usage of these new spacing variables
rg "var\(--tv-space-xx+l\)" --type css --type less
# Check for any hardcoded values that might need to be replaced with these variables
rg "(margin|padding).+?(?:20px|32px)" --type css --type less
Length of output: 8021
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
Release Notes
New Features
Improvements
Bug Fixes