-
Notifications
You must be signed in to change notification settings - Fork 330
fix(button): [button] Adapt to dark themes #3208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request updates the button component styles by modifying the CSS for handling disabled states and enhancing SVG icon fill colors. A new style for buttons with the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 (
|
WalkthroughThis pull request updates the button component to adapt to dark themes by modifying CSS variables and styles. It introduces new color variables for button states and adjusts the styling for icons and borders to ensure proper appearance in dark mode. Changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/theme/src/button/vars.less (1)
347-352: Refine Only-Icon Background and Hover VariablesThe updates introducing
--tv-Button-bg-color-only-icon(for the icon color in non-bordered only-icon buttons), and--tv-Button-bg-color-only-icon-ontext-hoveralong with--tv-Button-bg-color-only-icon-hover(for hover states)lend more granular control over the visual behavior of icon-only buttons. Please review the naming consistency and verify that these settings deliver the expected visual results under dark theme conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/theme/src/button/index.less(5 hunks)packages/theme/src/button/vars.less(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (8)
packages/theme/src/button/index.less (5)
5-6: Verify Prefix Variable InterpolationThe use of tilde (
~) for string interpolation in the definitions of@button-prefix-clsand@svg-prefix-clsensures that the values are treated as literal strings with dynamic prefixes. Please confirm that this syntax behaves consistently across your build environments and integrates well with dark theme adjustments.
226-229: Update Disabled State Styling for Dark ThemesIn the disabled state block, the explicit background-color property has been removed, leaving the styling to rely on the text, border, and icon color variables. This is in line with the objective to adapt the button styling for dark themes. Please verify that the new color values (set via
--tv-Button-text-color-disabled,--tv-Button-border-color-disabled, and--tv-Button-icon-color-disabled) provide sufficient contrast and accessibility in dark modes.
269-271: Set Default SVG Icon Color for Only-Icon ButtonsThe added rules for the default state of only-icon buttons—specifically setting the SVG fill with
var(--tv-Button-color-only-icon-default)—ensure that only-icon buttons use the newly defined variable. Please double-check that this color delivers proper visual contrast in dark themes.
277-279: Apply Hover Styling for Only-Icon ButtonsThe hover state for only-icon buttons now updates the SVG fill to use
var(--tv-Button-color-only-icon-default-hover). This change should help maintain consistent icon feedback when users hover over buttons. Ensure that the hover color works harmoniously within dark theme contexts.
343-349: Enhance Disabled-Only-Icon Text Button Hover BehaviorThe new styling block for
.@{button-prefix-cls}--text.is-disabledadds a hover rule that sets the SVG fill tovar(--tv-Button-disabled-color-only-icon-hover). This nicely differentiates the disabled state on hover. Please validate that this interaction style remains clear and accessible in both light and dark themes.packages/theme/src/button/vars.less (3)
332-335: Introduce New Only-Icon Default Color VariablesThe new variables
--tv-Button-color-only-icon-default--tv-Button-color-only-icon-default-hoverprovide dedicated control over the icon colors for only-icon buttons in their default and hover states. This change enhances theme adaptability. Ensure that these colors have been tested for adequate contrast in dark themes.
355-356: Define Disabled-Only-Icon Hover ColorThe variable
--tv-Button-disabled-color-only-icon-hovernow explicitly controls the icon color when a button is disabled and hovered. Confirm that this color meets accessibility contrast requirements, particularly in dark theme scenarios.
364-365: Update Mixed-Layout Icon ColorThe refreshed variable
--tv-Button-color-icon-svgis now in use to standardize the icon color across mixed-layout buttons. This change should ensure visual consistency. Please confirm its integration with dark theme palettes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/theme/src/button/vars.less (4)
334-337: Clarify Default Icon Color Commenting
The comments on these lines both state "仅图标且default主题时, 图标颜色." To improve clarity, consider explicitly distinguishing between the default state and the hover state. For example, one comment could mention "默认状态图标颜色" and the other "悬浮状态图标颜色."
347-352: Review Naming and Comment Consistency for Icon-Only Button Background Variables
The new variables—--tv-Button-bg-color-only-icon,--tv-Button-bg-color-only-icon-ontext-hover, and--tv-Button-bg-color-only-icon-hover—have comments that mix references to “图标色” and “背景色” in the hover state. Please verify that the variable names and comments accurately describe their intended roles (e.g., whether each is meant to control background color or the icon color for icon-only buttons when there is no border). Consider updating the comments or renaming variables if necessary for consistency.
355-356: Correct Typographical Error in Disabled Icon Color Comment
The comment on line 355 contains what appears to be a typo: "禁用事的图标色" should likely be "禁用时的图标色."
366-367: Update Comment for Mixed Layout Icon Variable
The comment on line 366 states "混排有边框和无边框时,图标右边距," which suggests a margin. However, the variable name--tv-Button-color-icon-svgindicates that it defines an icon color. Please update the comment to accurately reflect that it controls the icon color in mixed layouts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/theme/src/button/index.less(5 hunks)packages/theme/src/button/vars.less(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/button/index.less
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
fix(button): [button] Adapt to dark themes
Summary by CodeRabbit