fix(checkbox): [checkbox] fix checkbox error theme vars #2356
Conversation
WalkthroughThe changes in this pull request focus on updating the CSS custom properties for the checkbox component in the Changes
Possibly related PRs
Suggested labels
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: 0
🧹 Outside diff range and nitpick comments (2)
packages/theme/src/checkbox/vars.less (2)
22-23: LGTM. Consider adding a comment.The addition of
--tv-Checkbox-icon-selectedwithvar(--tv-color-bg-active-control)is a good improvement. It provides a specific variable for the selected state of the checkbox.Consider adding a brief comment above this line to explain the purpose of this variable, similar to the comments for other variables. For example:
// 复选框选中状态图标颜色 --tv-Checkbox-icon-selected: var(--tv-color-bg-active-control);
24-25: LGTM. Consider adding a comment.The addition of
--tv-Checkbox-icon-selected-disabledwithvar(--tv-color-bg-disabled-control-checked)is a good improvement. It provides a specific variable for the selected and disabled state of the checkbox.Consider adding a brief comment above this line to explain the purpose of this variable, similar to the comments for other variables. For example:
// 复选框选中禁用状态图标颜色 --tv-Checkbox-icon-selected-disabled: var(--tv-color-bg-disabled-control-checked);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/theme/src/checkbox/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/theme/src/checkbox/vars.less (3)
19-19: LGTM. Verify visual consistency.The change from
var(--tv-color-bg-control)tovar(--tv-color-border)for the checkbox icon color seems reasonable. It might improve contrast or align better with design guidelines.Please ensure this change:
- Aligns with the intended design.
- Maintains or improves accessibility (color contrast).
- Is visually consistent across different states of the checkbox.
21-21: LGTM. Consistent with previous change.The update from
var(--tv-color-icon-hover)tovar(--tv-color-border-hover)for the hover state is consistent with the change made to the non-hover state. This maintains the distinction between normal and hover states while aligning with the new color scheme.
19-26: Overall improvements look good. Clarify half-selected state handling.The changes to the checkbox variables improve the granularity of control over different states (normal, hover, selected, disabled). This should allow for more flexible and precise theming.
However, I noticed that comments related to the half-selected state have been removed. Could you please clarify:
- Has the half-selected state been removed from the checkbox component?
- If not, how is the half-selected state now being handled in terms of theming?
- Should we add variables for the half-selected state, or is it intentionally using styles from other states?
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