feat: optimize basic variables and switch the overall rounded corner … #2580
Conversation
…style of the component library
WalkthroughThe pull request introduces a new CSS variable 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 (
|
|
[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 (7)
packages/theme/src/dropdown/vars.less (1)
42-43: Consider single-line formatting for better readabilityWhile the border-radius property is functionally correct, consider combining the multi-line declaration into a single line for better maintainability and consistency with other variable declarations in this file.
- --tv-Dropdown-title-button-border-radius: var(--tv-border-radius-brand, 999px) 0 0 - var(--tv-border-radius-brand, 999px); + --tv-Dropdown-title-button-border-radius: var(--tv-border-radius-brand, 999px) 0 0 var(--tv-border-radius-brand, 999px);packages/theme/src/month-table/vars.less (1)
33-41: LGTM! Consider DRY optimizationThe border-radius updates are consistent across all directional variants. The changes maintain the existing functionality while modernizing the theming system.
Consider creating an intermediate variable to reduce repetition:
+ --tv-MonthTable-cell-border-radius-base: var(--tv-border-radius-brand, 999px); - --tv-MonthTable-cell-border-radius-start: var(--tv-border-radius-brand, 999px) 0 0 var(--tv-border-radius-brand, 999px); + --tv-MonthTable-cell-border-radius-start: var(--tv-MonthTable-cell-border-radius-base) 0 0 var(--tv-MonthTable-cell-border-radius-base);This would make future updates easier and reduce the chance of inconsistencies.
packages/theme/src/guide/vars.less (1)
Line range hint
1-60: Consider standardizing other border radius variablesThe file uses multiple border radius variables (
--tv-border-radius-lgfor headers, footers, and the shepherd element). Consider whether these should also be updated to use--tv-border-radius-brandfor consistency.packages/theme/src/date-table/vars.less (1)
Line range hint
34-80: Document this as a breaking visual changeThis change significantly alters the visual appearance of the date table component, switching from pill-shaped selections (border-radius: 999px) to slightly rounded corners (border-radius: 2px). This could impact the overall user experience and design consistency of applications using this component.
Consider:
- Adding this to the changelog as a breaking change
- Providing before/after screenshots in the PR description
- Adding a migration guide for users who want to maintain the previous rounded appearance
packages/theme/src/base/old-theme.less (1)
403-403: LGTM! Consider documenting the relationship with other border radius variables.The addition of
--tv-border-radius-brandaligns with the PR objective of switching rounded corner styles. The value of 2px is consistent with other border radius values in the theme.Consider expanding the comment to clarify:
- How this variable relates to other border radius variables like
--tv-border-radius-md- When to use this variable versus the other border radius options
- Whether this will become the new default for brand components
- --tv-border-radius-brand: 2px; // 品牌相关。用于圆角，按钮、分页等场景 + --tv-border-radius-brand: 2px; // 品牌相关组件的统一圆角值。作为默认圆角值(--tv-border-radius-md)的品牌特定替代项，用于按钮、分页等品牌相关场景。packages/theme/src/base/vars.less (2)
403-403: Consider using a more precise value for border-radius-roundThe comment correctly warns against using
50%to avoid oval shapes, but999pxis an arbitrary large value. Consider using a more precise value like9999pxwhich is a common practice, or calculating it based on the maximum possible element size.- --tv-border-radius-round: 999px; // 50% 会造成椭圆，避免使用。 + --tv-border-radius-round: 9999px; // 50% 会造成椭圆，避免使用。
404-404: Good addition of a semantic border-radius variableThe introduction of
--tv-border-radius-brandcreates a semantic layer of abstraction, making it easier to maintain consistent border radius styling across brand-related components. This follows the principle of separation of concerns by decoupling the technical implementation from its semantic usage.Consider documenting which components should use this brand-specific border radius versus the other border radius variables (
xs,sm,md,lg) to ensure consistent usage across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/theme/src/base/old-theme.less(1 hunks)packages/theme/src/base/vars.less(1 hunks)packages/theme/src/date-table/vars.less(3 hunks)packages/theme/src/dropdown/vars.less(1 hunks)packages/theme/src/guide/vars.less(1 hunks)packages/theme/src/month-table/vars.less(1 hunks)packages/theme/src/pager/vars.less(1 hunks)packages/theme/src/time-spinner/vars.less(1 hunks)
🔇 Additional comments (5)
packages/theme/src/time-spinner/vars.less (2)
39-39: LGTM! Variable update aligns with design system changes.
The change from --tv-border-radius-round to --tv-border-radius-brand maintains proper fallback handling while standardizing the border radius styling across components.
39-39: Verify the visual impact of the border radius change.
The change from a fully rounded appearance (999px) to a brand-specific radius (2px) is significant. Please ensure that:
- The time spinner items remain visually distinct and easily clickable
- The new radius maintains visual harmony with related components like dropdowns and date pickers
✅ Verification successful
Based on the search results, I can see that the border radius change affects the time spinner component, which is part of a larger date/time picker ecosystem. The search reveals that:
- The
--tv-border-radius-brandvariable is being used consistently across components, defaulting to--tv-border-radius-round(999px). - Related components like dropdowns, date pickers, and time pickers maintain visual consistency through this shared variable system.
Let me verify the specific styling implementation of the time spinner items:
Based on the code analysis, I can now provide a final response about the border radius change:
The border radius change from 999px to brand-specific radius is safe to implement
The time spinner items maintain their visual distinctiveness and clickability because:
- The items have sufficient padding and margin (defined by
--tv-TimeSpinner-item-paddingand--tv-TimeSpinner-item-margin) - Each item has a fixed width of 52px and clear hover/active states with background color changes
- The items are vertically stacked with good spacing, making them easily distinguishable regardless of border radius
The change to brand-specific radius (2px) aligns well with the design system as:
- The same variable (
--tv-border-radius-brand) is consistently used across related components - The time spinner is part of a cohesive date/time picker ecosystem that shares this styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find related components that might be affected by this border radius change
# Search for other components using the old border radius variable
rg --type less "tv-border-radius-round"
# Search for related time/date components that should maintain visual consistency
fd -e less -e vue . | grep -iE "time|date|picker|dropdown"
Length of output: 27445
Script:
#!/bin/bash
# Check the time spinner's styling implementation
cat packages/theme/src/time-spinner/index.less
Length of output: 3649
packages/theme/src/dropdown/vars.less (1)
39-40: Verify fallback value consistency with brand guidelines
The fallback value of 999px seems inconsistent with the new brand border radius of 2px mentioned in the PR summary. This could lead to unexpected visual differences when the brand variable is undefined.
Let's check for other occurrences of this pattern:
✅ Verification successful
The fallback value of 999px is consistent with the design system
The search results show that the fallback value of 999px is consistently used across multiple components (date-table, time-spinner, month-table, dropdown) when referencing --tv-border-radius-brand. This is intentional as:
- The base theme defines
--tv-border-radius-brand: var(--tv-border-radius-round)as the default - The old theme explicitly sets
--tv-border-radius-brand: 2px - The fallback of
999pxensures a pill-shaped appearance when neither theme variable is defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other border-radius-brand usages to verify consistency
rg --type less "border-radius-brand.*999px"
# Search for the definition of the brand border radius variable
rg --type less "tv-border-radius-brand"
Length of output: 4630
packages/theme/src/guide/vars.less (1)
47-47: Verify the new border radius variable dependency
The change from --tv-border-radius-round to --tv-border-radius-brand aligns with the PR's objective to standardize rounded corner styles. However, we should verify that the new variable is properly defined.
✅ Verification successful
The border radius variable change is properly defined and consistent
The verification shows that --tv-border-radius-brand is well-defined in two locations:
- In
base/vars.lessasvar(--tv-border-radius-round) - In
base/old-theme.lesswith a direct value of2px
Additionally, there are no remaining references to --tv-border-radius-round in the Guide component files, confirming a clean transition to the new variable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new border radius variable is defined and used consistently
# Expected: Find the definition of --tv-border-radius-brand and its usage across the codebase
# Search for the definition of the new variable
echo "Searching for --tv-border-radius-brand definition:"
rg --type less "tv-border-radius-brand:\s*" packages/
# Check for any remaining usage of the old variable in Guide-related files
echo -e "\nChecking for any remaining --tv-border-radius-round in Guide components:"
rg --type less "tv-border-radius-round" packages/theme/src/guide/
Length of output: 605
packages/theme/src/date-table/vars.less (1)
Line range hint 34-80: Verify visual consistency across all states
Please ensure the new border-radius works well with all date table states:
- Normal dates
- Selected dates
- Today's date
- Disabled dates
- Date range selection
- Hover states
✅ Verification successful
Let me gather more information about the date table component's visual states by searching for its implementation and related styles.
Based on the comprehensive analysis of the DateTable component's implementation and styles, I can now provide the final response:
Border radius implementation is consistent across all date table states ✓
The border-radius implementation is properly handled for all date table states:
- Normal dates use
--tv-DateTable-td-border-radius - Selected dates use
--tv-DateTable-border-radius-current-select - Today's date uses
--tv-DateTable-td-today-border-radius - Disabled dates use
--tv-DateTable-td-border-radius-disabled - Date range selection uses appropriate border radius for start/end dates
- Hover states use
--tv-DateTable-border-radius-current-select
All these border-radius variables are consistently set to var(--tv-border-radius-brand, 999px), ensuring visual consistency across all states. The implementation properly handles border-radius for both individual cells and range selections, including edge cases like first/last cells in a row.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any test files or visual regression tests related to the date table component
fd -e spec.js -e spec.ts -e test.js -e test.ts -e snap --exec grep -l "DateTable"
# Search for other components that might be using these variables
rg --type=less "tv-border-radius-round"
Length of output: 1420
Script:
#!/bin/bash
# Search for DateTable component implementation and styles
ast-grep --pattern 'class DateTable {
$$$
}'
# Search for DateTable styles
rg --type=less "DateTable" -A 3
# Search for any visual test snapshots of DateTable
fd -e snap --exec grep -l -A 5 "DateTable"
# Search for usage of the border radius variables in date table
rg "tv-DateTable.*border-radius" --type=less
Length of output: 23633
| @@ -28,17 +28,17 @@ | |||
| ) | |||
| 0; | |||
| // 单元格圆角（选中） | |||
| --tv-MonthTable-cell-border-radius: var(--tv-border-radius-round, 999px); | |||
| --tv-MonthTable-cell-border-radius: var(--tv-border-radius-brand, 999px); | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent border-radius variable usage detected across components
Several components are still using --tv-border-radius-round directly instead of --tv-border-radius-brand:
packages/theme/src/slider/vars.less: Slider handlepackages/theme/src/skeleton-item/vars.less: SkeletonItem circlepackages/theme/src/switch/vars.less: Switch innerpackages/theme/src/radio/vars.less: Radio innerpackages/theme/src/image-viewer/vars.less: ImageViewer actionspackages/theme/src/button/vars.less: Button round and circle variantspackages/theme/src/badge/vars.less: Badgepackages/theme/src/calendar-view/vars.less: CalendarView mark
For consistency with the MonthTable changes and the broader styling update, these components should also use --tv-border-radius-brand instead of directly referencing --tv-border-radius-round.
🔗 Analysis chain
LGTM! Verify visual consistency across states
The change from --tv-border-radius-round to --tv-border-radius-brand aligns with the PR objectives. The fallback value ensures backward compatibility.
Let's verify this change is consistent across the component library:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old border-radius variable
rg --type=less "tv-border-radius-round" packages/theme/src/
Length of output: 1358
| @@ -31,9 +31,9 @@ | |||
| // 表头文本色 | |||
| --tv-DateTable-th-text-color: var(--tv-color-text-weaken, #808080); | |||
| // 单元格圆角 | |||
| --tv-DateTable-td-border-radius: var(--tv-border-radius-round, 999px); | |||
| --tv-DateTable-td-border-radius: var(--tv-border-radius-brand, 999px); | |||
There was a problem hiding this comment.
Update fallback values to match the new design system
The changes switch from --tv-border-radius-round to --tv-border-radius-brand (which is 2px according to the PR description), but the fallback values are still set to 999px. This creates an inconsistent visual appearance when the brand variable is not available.
Apply this change to all affected border-radius variables:
- --tv-DateTable-td-border-radius: var(--tv-border-radius-brand, 999px);
+ --tv-DateTable-td-border-radius: var(--tv-border-radius-brand, 2px);
- --tv-DateTable-border-radius-current-select: var(--tv-border-radius-brand, 999px);
+ --tv-DateTable-border-radius-current-select: var(--tv-border-radius-brand, 2px);
- --tv-DateTable-td-today-border-radius: var(--tv-border-radius-brand, 999px);
+ --tv-DateTable-td-today-border-radius: var(--tv-border-radius-brand, 2px);
- --tv-DateTable-start-date-border-radius: var(--tv-border-radius-brand, 999px);
+ --tv-DateTable-start-date-border-radius: var(--tv-border-radius-brand, 2px);
- --tv-DateTable-cell-border-radius-selected: var(--tv-border-radius-brand, 999px);
+ --tv-DateTable-cell-border-radius-selected: var(--tv-border-radius-brand, 2px);
- --tv-DateTable-td-border-radius-disabled: var(--tv-border-radius-brand, 999px);
+ --tv-DateTable-td-border-radius-disabled: var(--tv-border-radius-brand, 2px);Also applies to: 36-36, 63-63, 74-74, 76-76, 80-80
| @@ -62,7 +62,7 @@ | |||
| // 分页页码项默认悬浮边框色 | |||
| --tv-Pager-page-item-border-color-hover: transparent; | |||
| // 分页页码项项圆角 | |||
| --tv-Pager-page-item-border-radius: var(--tv-border-radius-round); | |||
| --tv-Pager-page-item-border-radius: var(--tv-border-radius-brand); | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency found in border radius styling across components
Several components still use --tv-border-radius-round directly instead of --tv-border-radius-brand:
Switch: Inner border radius in switch/vars.lessSlider: Handle border radius in slider/vars.lessSkeletonItem: Circle border radius in skeleton-item/vars.lessRadio: Inner border radius in radio/vars.lessImageViewer: Actions border radius in image-viewer/vars.lessButton: Multiple border radius variables in button/vars.lessBadge: Border radius in badge/vars.lessCalendarView: Mark border radius in calendar-view/vars.less
This change should be part of a broader effort to standardize border radius styling using --tv-border-radius-brand. The current mix of direct round and brand usage could lead to inconsistent styling.
🔗 Analysis chain
LGTM! Verify consistency across components.
The change from --tv-border-radius-round to --tv-border-radius-brand aligns with the PR objective to switch the overall rounded corner style of the component library.
Let's verify that this change is consistent with other components:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of --tv-border-radius-round
# and confirm consistent usage of --tv-border-radius-brand across components
echo "Checking for any remaining --tv-border-radius-round usage..."
rg --type=less "border-radius-round" packages/theme/src/
echo "Verifying consistent --tv-border-radius-brand usage..."
rg --type=less "border-radius-brand" packages/theme/src/
Length of output: 4237
| --tv-border-radius-round: 999px; // 50% 会造成椭圆，避免使用。 | ||
| --tv-border-radius-brand: var(--tv-border-radius-round); // 品牌相关。用于圆角，按钮、分页等场景 |
There was a problem hiding this comment.
💡 Codebase verification
Consider updating border-radius declarations to use the new brand variable
Based on the verification results, we found:
- Multiple components using hardcoded
border-radius: 50%for circular elements (avatars, dots, icons) - Several mobile components using hardcoded
border-radius: 999pxthat could benefit from the new variable - Limited adoption of
--tv-border-radius-brand(only in pager and guide components)
Suggested updates:
- Keep
border-radius: 50%for truly circular elements like avatars and dots - Update the following components to use
--tv-border-radius-brand:- Mobile button component (
packages/theme-mobile/src/button/index.less) - Mobile switch component (
packages/theme-mobile/src/switch/index.less) - Mobile radio component (
packages/theme-mobile/src/radio/index.less) - Mobile numeric component (
packages/theme-mobile/src/numeric/index.less)
- Mobile button component (
🔗 Analysis chain
Verify consistent border radius usage across components
Let's ensure the new brand border radius is being applied consistently and there aren't any hardcoded border radius values that should be using this variable instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for hardcoded border radius values and current usage of the new variable
# Check for hardcoded border radius values that might need to be replaced
echo "Checking for hardcoded border radius values:"
rg -g '*.{less,css,vue}' '(border-radius:\s*999px|border-radius:\s*50%)'
# Check current usage of the new brand border radius variable
echo -e "\nChecking usage of new brand border radius variable:"
rg -g '*.{less,css,vue}' 'var\(--tv-border-radius-brand\)'
Length of output: 6277
…style of the component library
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
--tv-border-radius-brand, enhancing theming capabilities across components.Bug Fixes