feat(pager): [pager] adapt to pager old theme #2531
Conversation
WalkthroughThe changes in this pull request primarily focus on modifying CSS variables related to border radius and styling in the theme and pager components. A new variable 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: 1
🧹 Outside diff range and nitpick comments (4)
packages/theme/src/pager/vars.less (1)
72-75: Consider differentiating hover and active text colorsBoth
--tv-Pager-page-item-text-color-hoverand--tv-Pager-page-item-text-color-activeuse the same valuevar(--tv-color-text). This might not provide enough visual distinction between states.Consider using different color variables to better distinguish between hover and active states:
// 分页页码选中文本色 --tv-Pager-page-item-text-color-active: var(--tv-color-text); // 分页页码悬浮文本色 - --tv-Pager-page-item-text-color-hover: var(--tv-color-text); + --tv-Pager-page-item-text-color-hover: var(--tv-color-text-hover);packages/theme/src/pager/index.less (1)
418-426: LGTM! Consider using CSS custom property for calculationsThe mini variant's sizing is well-implemented. Consider extracting the common calculation pattern into a CSS custom property for better maintainability.
+/* At the top of the file with other variable declarations */ +--tv-Pager-line-height-offset: 2px; -line-height: calc(var(--tv-Pager-input-height-xs) - 2px); +line-height: calc(var(--tv-Pager-input-height-xs) - var(--tv-Pager-line-height-offset));packages/theme/src/button/vars.less (1)
Line range hint
1-24: Consider improving variable organization and documentation.The button variables section could benefit from better organization and documentation:
- Add a comment block explaining the relationship between
--tv-border-radius-brandand other radius variables- Consider grouping border radius variables together for better maintainability
- Add examples or design tokens documentation to clarify the visual differences between radius values
// --------------------------------------------------- 场景1、圆角-------------------------------------------- + // Border radius hierarchy: + // --tv-border-radius-brand: Default brand-specific radius (used for buttons) + // --tv-border-radius-round: Full rounded corners + // --tv-border-radius-circle: Circular shape (50%) + // 默认时按钮圆角 --tv-Button-border-radius: var(--tv-border-radius-brand); // 默认规范不需要 6px的圆角的效果，默认就是半圆packages/theme/src/base/old-theme.less (1)
Line range hint
1-393: Consider enhancing theme system documentation and validationWhile the theme structure is well-organized, consider the following improvements:
- Add a README.md file documenting:
- The theme system architecture
- Guidelines for adding new variables
- Migration paths between themes
- Implement theme validation to ensure:
- Consistent variable naming
- Complete coverage of all required design tokens
- No duplicate or conflicting values
Would you like me to help create a theme documentation template or validation script?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/theme/src/base/old-theme.less(1 hunks)packages/theme/src/base/vars.less(1 hunks)packages/theme/src/button/vars.less(1 hunks)packages/theme/src/pager/index.less(3 hunks)packages/theme/src/pager/vars.less(1 hunks)
🔇 Additional comments (5)
packages/theme/src/pager/index.less (2)
Line range hint 93-99: LGTM! Improved hover state variable naming
The hover state styling now uses more semantic and specific variable names, which better reflects their purpose and scope within the pager component.
119-122: Verify border color variable reuse
The active state is using the hover state's border color variable (--tv-Pager-page-item-border-color-hover). While this might be intentional for visual consistency, consider if it should have its own dedicated variable for better maintainability and flexibility.
-border: 1px solid var(--tv-Pager-page-item-border-color-hover);
+border: 1px solid var(--tv-Pager-page-item-border-color-active);packages/theme/src/button/vars.less (1)
23-23: Verify the impact of border radius changes across components.
The change from --tv-border-radius-round to --tv-border-radius-brand could affect the visual consistency across components. This appears to be a breaking change that modifies the default button appearance.
Let's verify the border radius usage across components:
Consider:
- Documenting this change in the migration guide if this is a breaking change
- Ensuring all components that should share the same border radius are updated to use
--tv-border-radius-brand
✅ Verification successful
Border radius change is consistent with design system migration
Based on the comprehensive analysis of border radius usage across the component library:
-
The change from
--tv-border-radius-roundto--tv-border-radius-brandin button component aligns with the design system, as evidenced by:- Other components like
Pageralready use--tv-border-radius-brand - Components that need circular/round styling (like Radio, Switch) explicitly use
--tv-border-radius-round - The comment indicates this is an intentional change to match default specifications
- Other components like
-
The change is localized to button's default border radius and doesn't affect:
- Components that need circular styling (they use dedicated variables)
- Components with their own border radius definitions
- Button variants that need specific border radius values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other components using border radius variables
# to ensure consistent theming across the component library
echo "Checking border radius usage across components..."
# Search for border radius variables in theme files
rg --type=less "border-radius: var\(--tv-border-radius-(round|brand)\)" packages/theme/src/
# Search for potential visual inconsistencies with other components
rg --type=less "border-radius:" packages/theme/src/ | grep -v "brand\|round"
Length of output: 33244
packages/theme/src/base/old-theme.less (1)
392-393: LGTM! The new border radius variables align with the theme system.
The addition of --tv-border-radius-default and --tv-border-radius-brand variables with consistent values (var(--tv-border-radius-xs)) helps maintain design consistency across components.
Let's verify the usage of these new variables across the codebase:
✅ Verification successful
The new border radius variables are correctly used across the codebase
The verification shows that:
--tv-border-radius-brandis used in key components like Button, Pager, etc.--tv-border-radius-defaultis widely used across many components like Input, Textarea, Upload, etc.- The hardcoded border-radius values found are mostly for specific design requirements (like circular shapes with 999px) or internal component elements, not replacing these theme variables.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of the new border radius variables
# Search for usage of border-radius-brand
rg --type=less --type=css -A 2 "border-radius-brand"
# Search for usage of border-radius-default
rg --type=less --type=css -A 2 "border-radius-default"
# Look for any potential hardcoded border radius values that should use these variables
rg --type=less --type=css "border-radius:\s+[0-9]+px"
Length of output: 24524
packages/theme/src/base/vars.less (1)
394-394: LGTM! Verify usage in affected components.
The new CSS variable --tv-border-radius-brand is well-defined and properly documented. It helps maintain consistent branding across components by using the round border radius.
Let's verify the usage of this new variable across components:
✅ Verification successful
Verified: Border radius changes properly implemented across components
The new --tv-border-radius-brand variable is correctly:
- Used in pager component via
--tv-Pager-page-item-border-radius - Applied to button component via
--tv-Button-border-radius - Properly switches between round (new theme) and 2px (old theme) values
- Maintains separate border radius for inputs in pager using
--tv-border-radius-default
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where the new border radius variable is being used
# Expected: Usage in pager and other brand-related components
# Search for the new variable usage
rg --type=less --type=css -A 2 "tv-border-radius-brand"
# Search for potential places where it should be used (border-radius properties in pager)
rg --type=less --type=css -A 2 "border-radius.*pager"
Length of output: 1067
Script:
#!/bin/bash
# Let's verify the actual implementation and usage in pager component
# First, check the pager component's style implementation
rg --type=less -A 5 "\.Pager" packages/theme/src/pager/
# Then, check if there are any other border-radius related variables in pager
rg --type=less "border-radius" packages/theme/src/pager/
# Finally, check if the old theme's border radius value is being used correctly
rg --type=less "tv-border-radius-xs" packages/theme/src/base/
Length of output: 1107
| @@ -64,13 +62,17 @@ | |||
| // 分页页码项默认悬浮边框色 | |||
| --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
Border radius inconsistency detected in pager component
The pager component's border radius (--tv-border-radius-brand) is inconsistent with the old theme's design system. In the old theme (base/old-theme.less), --tv-border-radius-brand is set to --tv-border-radius-xs, while in the default theme it's set to --tv-border-radius-round. This change should be reverted to maintain consistency with the old theme's design pattern.
packages/theme/src/base/old-theme.less:--tv-border-radius-brand: var(--tv-border-radius-xs)packages/theme/src/pager/vars.less: Currently using--tv-border-radius-brandwhich resolves to--tv-border-radius-roundin default theme
🔗 Analysis chain
Verify theme-wide border radius consistency
The change from --tv-border-radius-round to --tv-border-radius-brand aligns with the PR objective. However, let's ensure this change is consistent with other components in the old theme.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistencies in border radius usage across theme files
# Look for any remaining usage of --tv-border-radius-round that should be updated
# Search for border radius variables in theme files
rg --type=less "border-radius-(?:round|brand)" packages/theme/src/
Length of output: 3986
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/button/vars.less (1)
23-23: Consider documenting the border radius value.The inline comment mentions "6px" but this value isn't explicitly documented. Consider adding a comment specifying the actual value of
--tv-border-radius-brandfor better maintainability.- --tv-Button-border-radius: var(--tv-border-radius-brand); // 默认规范不需要 6px的圆角的效果，默认就是半圆 + --tv-Button-border-radius: var(--tv-border-radius-brand); // Using brand border radius (defined as XXpx) instead of round corners
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/theme/src/base/old-theme.less(1 hunks)packages/theme/src/base/vars.less(1 hunks)packages/theme/src/button/vars.less(1 hunks)packages/theme/src/pager/index.less(3 hunks)packages/theme/src/pager/vars.less(1 hunks)
🔇 Additional comments (7)
packages/theme/src/pager/vars.less (1)
65-65: Verify the visual impact of border radius change
The change from --tv-border-radius-round to --tv-border-radius-brand aligns with the PR objective of adapting to the old theme. However, we should ensure this change maintains visual consistency with other components using the brand radius.
✅ Verification successful
Border radius change is consistent with design system
The change from --tv-border-radius-round to --tv-border-radius-brand is verified to be consistent with the design system:
- The brand radius is already used by other key components like buttons
- In base theme, it defaults to round radius (
--tv-border-radius-round) - In old theme, it's set to extra small radius (
--tv-border-radius-xs) - This change maintains visual consistency across components in both themes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using the brand radius to ensure consistency
rg --type=less "border-radius-brand" packages/theme/src/
Length of output: 515
packages/theme/src/pager/index.less (3)
Line range hint 93-99: LGTM: Hover state styling is well-structured
The hover state implementation is comprehensive and follows best practices by:
- Using specific selectors to target non-dot and non-active items
- Properly handling both text and icon colors
- Including transition properties for smooth state changes
119-122: Verify the intentional reuse of hover border color for active state
The active state is using var(--tv-Pager-page-item-border-color-hover) instead of a dedicated active state border color variable. While this might be intentional for maintaining visual consistency, consider if a separate active state border color variable would be more appropriate for better theme customization.
Consider creating a dedicated variable for active state border color:
- border: 1px solid var(--tv-Pager-page-item-border-color-hover);
+ border: 1px solid var(--tv-Pager-page-item-border-color-active);418-426: LGTM: Mini variant styling is well-implemented
The mini variant implementation:
- Correctly uses the xs-specific variables for consistent sizing
- Properly calculates height and line-height
- Maintains consistent styling with the rest of the component
packages/theme/src/button/vars.less (1)
23-23: Verify the new border radius variable's existence and impact.
The change from --tv-border-radius-round to --tv-border-radius-brand appears to align with the old theme adaptation. However, we should ensure this change maintains visual consistency.
Let's verify the new variable's definition and usage:
✅ Verification successful
Border radius variable change is properly defined and consistently applied
The verification shows that:
- The
--tv-border-radius-brandis properly defined inbase/vars.lessand overridden inold-theme.less - Only buttons and pager components use the brand-specific radius, which is intentional
- Other components (switch, slider, badge, etc.) maintain their round radius, which is appropriate as they are not brand-specific elements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the definition and usage of the new border radius variable
# Check if the new variable is defined in base theme files
echo "Checking for variable definition..."
rg --type less "tv-border-radius-brand"
# Check for potential visual inconsistencies with other components
echo "Checking other components using border radius..."
rg --type less "border-radius: var\(--tv-border-radius-(round|brand)\)"
Length of output: 1833
packages/theme/src/base/old-theme.less (1)
392-393: LGTM! The border radius variables are properly defined.
The addition of --tv-border-radius-brand and update to --tv-border-radius-default align well with the PR objective of adapting the pager component to the old theme. Both variables correctly reference --tv-border-radius-xs (2px).
Let's verify the usage of these variables in related components:
✅ Verification successful
Border radius variables are correctly used in components
The verification confirms that the border radius variables are properly integrated:
--tv-border-radius-brandis correctly used in brand-related components:- Pager component's page items (
--tv-Pager-page-item-border-radius) - Button component's main radius (
--tv-Button-border-radius)
- Pager component's page items (
--tv-border-radius-defaultis widely used across various components for standard UI elements like inputs, panels, and containers
The changes align with the old theme adaptation and maintain consistency with the existing component styles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of the new border radius variables in component styles
# Search for border radius variable usage
rg -t less "tv-border-radius-(brand|default)" packages/theme/src/
# Check specific component styles that might be affected
rg -t less "border-radius" packages/theme/src/pager/
rg -t less "border-radius" packages/theme/src/button/
Length of output: 8065
packages/theme/src/base/vars.less (1)
394-394: LGTM! Verify the variable usage across components.
The addition of --tv-border-radius-brand with the value of --tv-border-radius-round is appropriate for maintaining consistent branding across components.
Let's verify the usage of this new variable in related components:
✅ Verification successful
Variable is correctly used in button and pager components
The new --tv-border-radius-brand variable is properly integrated:
- Used in
packages/theme/src/pager/vars.lessfor page item border radius - Used in
packages/theme/src/button/vars.lessfor button border radius - Has fallback in
packages/theme/src/base/old-theme.lessusing--tv-border-radius-xs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of the new border radius variable in button and pager components
# Search for the new variable usage
rg --type=less "tv-border-radius-brand"
# Check specific component files that might use this variable
fd -e less -e vue "button|pager" packages/theme/src/
Length of output: 633
| // 分页页码选中文本色 | ||
| --tv-Pager-page-item-text-color-active: var(--tv-color-text); | ||
| // 分页页码悬浮文本色 | ||
| --tv-Pager-page-item-text-color-hover: var(--tv-color-text); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of both the component-specific color patterns and the base theme variables, I can now provide a final response:
Consider differentiating hover and active text colors
The current implementation using var(--tv-color-text) (#191919) for both hover and active states differs from the established pattern in other interactive components. For example:
- Anchor component uses secondary text color (#595959) for hover and primary text color (#191919) for active
- Tabs use different colors for hover (#ffffff) and active (#191919) states
- Dropdown items use a specific active color (#1476ff)
Consider using:
var(--tv-color-text-secondary)for hover statevar(--tv-color-text-active)for active state
This would provide better visual feedback and maintain consistency with other components in the theme.
🔗 Analysis chain
Review the text color implementation
The new text color variables for active and hover states both use var(--tv-color-text). This creates a consistent text color behavior but might reduce visual feedback for user interactions. Consider if this matches the intended design in the old theme.
Let me analyze the hover and active text color patterns in other components to make a proper assessment. I'll need to check the actual color values being used.
Let me try a different approach to check the color variables in the theme's root.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other interactive components in the old theme use similar color patterns
rg --type=less "text-color-active|text-color-hover" packages/theme/src/
Length of output: 2830
Script:
#!/bin/bash
# Check the color variables being used for text colors in interactive states
rg --type=less "color-text|color-text-active|color-text-secondary|color-text-weaken" packages/theme/src/vars/
Length of output: 176
Script:
#!/bin/bash
# Check the base color variables in the theme
rg --type=less "^[[:space:]]*--tv-color-text" packages/theme/src/
Length of output: 2560
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
--tv-border-radius-brandfor enhanced branding in buttons and pagination components.Updates
Improvements