refactor(theme): [slider] refactor slider theme vars #2295
Conversation
WalkthroughThe pull request introduces modifications to the 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 (1)
packages/theme/src/slider/vars.less (1)
59-59: Consider using a relative unit for--tv-Slider-input-widthOn line 59,
--tv-Slider-input-widthis set to80px. For better scalability and responsiveness across different devices, consider using a relative unit likeemorrem. This allows the input width to adjust based on the root font size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/theme/src/base/vars.less (1 hunks)
- packages/theme/src/slider/index.less (8 hunks)
- packages/theme/src/slider/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/theme/src/base/vars.less (2)
Line range hint
1-597: Well-organized and comprehensive design token system.The
vars.lessfile presents a well-structured and comprehensive set of design tokens. The organization into categories such as colors, typography, spacing, and shadows provides a solid foundation for maintaining consistency across the application. This structure will facilitate easier updates and maintenance of the design system in the future.
297-297: Approve color change for disabled control icons, but verify consistency.The color for
--tv-color-icon-control-disabledhas been updated from#1476ffto#dbdbdb. This lighter color is more appropriate for a disabled state, improving visual feedback for users. However, please ensure this change is intentional and consistent with the overall design system.To ensure consistency, let's check if there are any other occurrences of the old color value that might need updating:
packages/theme/src/slider/vars.less (2)
82-82: Verify the reference for--tv-Slider-tips-border-colorOn line 82,
--tv-Slider-tips-border-coloris set tovar(--tv-Slider-tips-bg-color). If the intention is for the border color to match the background color of the tips, this is acceptable. However, if a distinct border color is desired, please verify and update the reference accordingly.
15-84: Consistency in variable naming and usageThe refactoring updates variable prefixes from
--ti-to--tv-, aligning with the new naming convention. Excellent work ensuring that all variables follow this convention consistently.packages/theme/src/slider/index.less (1)
Line range hint
21-248: CSS Variable Refactoring Enhances ConsistencyThe refactoring of CSS variables to the new
--tv-Slider-prefix significantly improves the consistency and maintainability of the codebase. The updated variable names are clear and adhere to the established naming convention, ensuring that styles are easier to manage and understand.
| --ti-slider-range-top: calc(-1 * var(--ti-common-space-base)); | ||
| // 滑块进度条顶部外边距 | ||
| --ti-slider-range-margin-top: 0; | ||
| --tv-Slider-range-top: -4px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust --tv-Slider-range-top for better alignment
On line 32, --tv-Slider-range-top is set to -4px. Ensure that this negative value correctly aligns the slider range with the handle. It might be beneficial to calculate this value based on the slider height or handle dimensions to maintain consistency if those sizes change.
| --tv-Slider-handle-border-color: var(--tv-color-border-disabled); | ||
| // 滑块点圆角 | ||
| --ti-slider-handle-border-radius: var(--ti-common-border-radius-5, 12px); | ||
| --tv-Slider-handle-border-radius: var(--tv-border-radius-round); | ||
|
|
||
| // 滑块点悬浮文本色 | ||
| --ti-slider-handle-text-color-hover: var(--ti-common-color-bg-hover, #595959); | ||
| --tv-Slider-handle-text-color-hover: var(--tv-color-text-secondary); | ||
| // 滑块点悬浮边框色 | ||
| --ti-slider-handle-border-color-hover: var(--ti-common-color-data-1, #1476ff); | ||
| --tv-Slider-handle-border-color-hover: var(--tv-color-border-active-control); | ||
| // 滑块点阴影 | ||
| --ti-slider-handle-box-shadow: 0 1px 4px 0 rgba(0, 0, 0, 0.08); | ||
| // 滑块点top | ||
| --ti-slider-handle-top: calc(-1 * var(--ti-common-space-base)); | ||
| // 滑块点边框厚度 | ||
| --ti-slider-handle-border-weight: var(--ti-common-border-weight-normal, 1px); | ||
| // 滑块点边框悬浮厚度 | ||
| --ti-slider-handle-border-weight-hover: var(--ti-common-border-weight-1, 2px); | ||
| // 滑块点图标的display(hide) | ||
| --ti-slider-handle-icon-display: none; | ||
| // 滑块点图标色 | ||
| --ti-slider-handle-icon-fill-color: var(--ti-common-color-data-1, #1476ff); | ||
| // 滑块点图标悬浮色 | ||
| --ti-slider-handle-icon-fill-color-hover: var(--ti-common-color-bg-hover, #595959); | ||
| // 滑块点的缩放值(hide) | ||
| --ti-slider-handle-transform: scale(1); | ||
| // 滑块点顶部外边距 | ||
| --ti-slider-handle-margin-top: calc(var(--ti-common-space-2x, 8px) * -1); | ||
| // 滑块点水平外边距 | ||
| --ti-slider-handle-margin-horizontal: calc(var(--ti-common-space-2x, 8px) * -1); | ||
| // 滑块点底部外边距 | ||
| --ti-slider-handle-margin-bottom: var(--ti-common-space-0, 0px); | ||
| // 滑块点禁用时图标颜色 | ||
| --ti-slider-handle-icon-disabled-fill-color: var(--ti-common-color-icon-disabled, #c2c2c2); | ||
| --tv-Slider-handle-box-shadow: var(--tv-shadow-1-down); | ||
| // 滑块点禁用时边框颜色 | ||
| --ti-slider-handle-disabled-border-color: var(--ti-common-color-icon-disabled, #c2c2c2); | ||
| --tv-Slider-handle-disabled-border-color: var(--tv-color-border); |
There was a problem hiding this comment.
Inconsistent assignment of border colors for slider handle
It appears that the border colors for the slider handle in its default and disabled states might be inverted:
- Line 43:
--tv-Slider-handle-border-coloris set tovar(--tv-color-border-disabled), which suggests the handle is in a disabled state. - Line 54:
--tv-Slider-handle-disabled-border-coloris set tovar(--tv-color-border), which is typically used for enabled elements.
To maintain consistency and ensure the correct visual representation, consider swapping the variable assignments:
- --tv-Slider-handle-border-color: var(--tv-color-border-disabled);
+ --tv-Slider-handle-border-color: var(--tv-color-border);
...
- --tv-Slider-handle-disabled-border-color: var(--tv-color-border);
+ --tv-Slider-handle-disabled-border-color: var(--tv-color-border-disabled);📝 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-Slider-handle-border-color: var(--tv-color-border-disabled); | |
| // 滑块点圆角 | |
| --ti-slider-handle-border-radius: var(--ti-common-border-radius-5, 12px); | |
| --tv-Slider-handle-border-radius: var(--tv-border-radius-round); | |
| // 滑块点悬浮文本色 | |
| --ti-slider-handle-text-color-hover: var(--ti-common-color-bg-hover, #595959); | |
| --tv-Slider-handle-text-color-hover: var(--tv-color-text-secondary); | |
| // 滑块点悬浮边框色 | |
| --ti-slider-handle-border-color-hover: var(--ti-common-color-data-1, #1476ff); | |
| --tv-Slider-handle-border-color-hover: var(--tv-color-border-active-control); | |
| // 滑块点阴影 | |
| --ti-slider-handle-box-shadow: 0 1px 4px 0 rgba(0, 0, 0, 0.08); | |
| // 滑块点top | |
| --ti-slider-handle-top: calc(-1 * var(--ti-common-space-base)); | |
| // 滑块点边框厚度 | |
| --ti-slider-handle-border-weight: var(--ti-common-border-weight-normal, 1px); | |
| // 滑块点边框悬浮厚度 | |
| --ti-slider-handle-border-weight-hover: var(--ti-common-border-weight-1, 2px); | |
| // 滑块点图标的display(hide) | |
| --ti-slider-handle-icon-display: none; | |
| // 滑块点图标色 | |
| --ti-slider-handle-icon-fill-color: var(--ti-common-color-data-1, #1476ff); | |
| // 滑块点图标悬浮色 | |
| --ti-slider-handle-icon-fill-color-hover: var(--ti-common-color-bg-hover, #595959); | |
| // 滑块点的缩放值(hide) | |
| --ti-slider-handle-transform: scale(1); | |
| // 滑块点顶部外边距 | |
| --ti-slider-handle-margin-top: calc(var(--ti-common-space-2x, 8px) * -1); | |
| // 滑块点水平外边距 | |
| --ti-slider-handle-margin-horizontal: calc(var(--ti-common-space-2x, 8px) * -1); | |
| // 滑块点底部外边距 | |
| --ti-slider-handle-margin-bottom: var(--ti-common-space-0, 0px); | |
| // 滑块点禁用时图标颜色 | |
| --ti-slider-handle-icon-disabled-fill-color: var(--ti-common-color-icon-disabled, #c2c2c2); | |
| --tv-Slider-handle-box-shadow: var(--tv-shadow-1-down); | |
| // 滑块点禁用时边框颜色 | |
| --ti-slider-handle-disabled-border-color: var(--ti-common-color-icon-disabled, #c2c2c2); | |
| --tv-Slider-handle-disabled-border-color: var(--tv-color-border); | |
| --tv-Slider-handle-border-color: var(--tv-color-border); | |
| // 滑块点圆角 | |
| --tv-Slider-handle-border-radius: var(--tv-border-radius-round); | |
| // 滑块点悬浮文本色 | |
| --tv-Slider-handle-text-color-hover: var(--tv-color-text-secondary); | |
| // 滑块点悬浮边框色 | |
| --tv-Slider-handle-border-color-hover: var(--tv-color-border-active-control); | |
| // 滑块点阴影 | |
| --tv-Slider-handle-box-shadow: var(--tv-shadow-1-down); | |
| // 滑块点禁用时边框颜色 | |
| --tv-Slider-handle-disabled-border-color: var(--tv-color-border-disabled); |
| position: absolute; | ||
| padding: 12px 16px; | ||
| line-height: 1.5; | ||
| z-index: 1010; | ||
| word-break: normal; // popup-body里面加了默认折行影响到此处需要重置 | ||
| white-space: nowrap; // 强制不换行-to liyingfu 纵向进度条上的tips如果内容过多并有空格就会掉下去，此处设置强制显示一行 | ||
| background: var(--ti-slider-tips-bg-color); | ||
| border: 1px solid var(--ti-slider-tips-border-color); | ||
| background: var(--tv-Slider-tips-bg-color); | ||
| border-radius: var(--ti-common-border-radius-1); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Variable Naming Detected for border-radius
Multiple definitions of --ti-common-border-radius-1 found with differing values. This inconsistency can lead to theming issues and confusion in variable management.
- Found in:
packages/theme/src/slider/index.lessat line 174
🔗 Analysis chain
Inconsistent Variable Naming for border-radius
At line 174, the border-radius is set using var(--ti-common-border-radius-1), which differs from the updated --tv-Slider- prefix convention used elsewhere in the file. This inconsistency might lead to theming issues or confusion in variable management.
To confirm whether --ti-common-border-radius-1 should be updated to match the new naming convention or if it's intentionally left as a common global variable, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the definition and usage of `--ti-common-border-radius-1`
# Expected Outcome: Determine if `--ti-common-border-radius-1` is a global variable used across multiple components.
# Search for the variable definition in LESS files
rg --type less --no-filename --no-line-number --pretty '^\s*--ti-common-border-radius-1\s*:'
# Search for usages of the variable in LESS files
rg --type less --pretty 'var\(--ti-common-border-radius-1\)'
Length of output: 340
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
Chores