fix(radio): [radio,select,tree-select,checkbox,radio,button] fix the style of dark themes #3237
Conversation
WalkthroughThis pull request addresses issues related to the dark theme in the Tiny Vue project. It includes changes to the button, checkbox, option, radio, and tree components, focusing on improving the visual consistency and functionality of these components in dark mode. Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| @@ -111,12 +111,14 @@ | |||
| &:active, | |||
| &:focus, | |||
| &:hover, | |||
| &.is-active { | |||
| &.is-active, | |||
| &.is-loading.is-loading.is-loading.is-loading.is-loading { | |||
There was a problem hiding this comment.
The repeated class .is-loading.is-loading.is-loading.is-loading.is-loading seems to be a mistake. It should be corrected to a single .is-loading to avoid redundancy and potential issues.
| @@ -221,9 +223,8 @@ | |||
|
|
|||
| // 禁用态+loading 优先级最高， 五优先级 | |||
| &.is-disabled.is-disabled.is-disabled.is-disabled.is-disabled, | |||
There was a problem hiding this comment.
The repeated class .is-disabled.is-disabled.is-disabled.is-disabled.is-disabled should be corrected to a single .is-disabled to avoid redundancy and potential issues.
|
[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 |
MomoPoppy
changed the title
WalkthroughThis pull request introduces additional loading button instances in two Vue demo files and refines the styling across various components within the theme package. The changes include new CSS variables, updated color definitions, and adjustments to SVG fill properties and padding for buttons, checkboxes, options, radio buttons, and tree nodes. Overall, the modifications improve the visual presentation of loading states and disabled elements without altering any exported or public JavaScript interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VueComponent
participant TinyButton
User->>VueComponent: Request loading page
VueComponent->>TinyButton: Render first tiny-button (loading state)
VueComponent->>TinyButton: Render second tiny-button without type attribute
User->>VueComponent: View updated buttons with refined styling
Possibly related PRs
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
examples/sites/demos/pc/app/checkbox/basic-usage.spec.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. examples/sites/demos/pc/app/button/dynamic-disabled.spec.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🪧 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
🧹 Nitpick comments (3)
packages/theme/src/tree/index.less (2)
238-238: Update tree node connection line positioningThe calculation for the
leftproperty in the pseudo-element now usesvar(--tv-Tree-node-padding-x)instead of the previous padding variable. This change promotes consistency with the updated naming convention for tree node padding.
252-252: Standardize content indent offsetSimilarly, the
leftvalue in the.content-indent::beforeblock has been updated to usevar(--tv-Tree-node-padding-x). Please verify that all related components are aligned with this new variable.packages/theme/src/button/index.less (1)
347-349: Icon-Only Button Hover Color Update
The updated hover styling for icon-only buttons uses the variablevar(--tv-Button-bg-color-only-icon-hover)to set the SVG fill on hover. This change should help improve the visual feedback in dark theme scenarios.
Please double-check that this variable provides adequate contrast and aligns with the design guidelines for dark mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
examples/sites/demos/pc/app/button/loading-composition-api.vue(1 hunks)examples/sites/demos/pc/app/button/loading.vue(1 hunks)packages/theme/src/base/dark-theme.less(2 hunks)packages/theme/src/button/index.less(6 hunks)packages/theme/src/button/vars.less(4 hunks)packages/theme/src/checkbox/index.less(1 hunks)packages/theme/src/option/index.less(2 hunks)packages/theme/src/option/vars.less(2 hunks)packages/theme/src/radio/vars.less(1 hunks)packages/theme/src/tree/index.less(3 hunks)packages/theme/src/tree/vars.less(1 hunks)
🔇 Additional comments (26)
examples/sites/demos/pc/app/button/loading.vue (1)
4-6: UI Enhancement: Consistent Spacing in Loading StateThe addition of two
<br />elements and a secondary<tiny-button>(without an explicit type) creates better visual separation during loading. Please verify that the resulting spacing meets the design requirements for the dark theme.examples/sites/demos/pc/app/button/loading-composition-api.vue (1)
4-6: UI Enhancement in Composition API TemplateIntroducing two
<br />elements followed by an additional<tiny-button>(without thetypeattribute) mirrors the changes in the standard template. This helps ensure consistency across different Vue API styles while maintaining the appropriate loading state visuals.packages/theme/src/tree/vars.less (1)
65-66: CSS Variable Renaming: Tree Node Padding UpdateThe variable previously used for node left padding appears to have been renamed to
--tv-Tree-node-padding-x, reflecting a shift to a generalized horizontal padding approach. Please ensure that all component styles referencing the old variable are updated accordingly to prevent inconsistencies.packages/theme/src/radio/vars.less (1)
36-37: CSS Variable Update: Radio Button Disabled Border ColorThe update to
--tv-Radio-input-checked-disabled-border-colornow referencesvar(--tv-color-icon-checked-disabled, #dbdbdb), which should help standardize the appearance of disabled radio buttons, especially under dark theme conditions. Confirm that all related components properly reflect this adjusted styling.packages/theme/src/option/vars.less (1)
28-34: CSS Variables Update: Checkbox Hover State StylingA new variable
--tv-Option-icon-color-hoverhas been introduced to control the hover color for checkbox icons, effectively replacing the older hover styling mechanism (previously managed by--tv-Option-unchecked-border-color-hover). This change enhances semantic clarity and should improve consistency across UI interactions. Please verify that any legacy styles dependent on the old variable are correctly migrated to the new system.packages/theme/src/tree/index.less (1)
360-360: Adjust horizontal padding using new variableThe padding now uses
padding: 0 var(--tv-Tree-node-padding-x);, which aligns the horizontal spacing with the new padding variable. This improves overall layout consistency.packages/theme/src/checkbox/index.less (7)
153-158: Refine inner input SVG sizingThe SVG icons within the
.@{checkbox-prefix-cls}__innerblock now correctly adopt the font size fromvar(--tv-Checkbox-icon-size). This change ensures consistent icon sizing across checkboxes.
159-166: Update fill for checked and half-selected statesThe fill for
.icon-checked-surand.icon-halfselectnow usesvar(--tv-Checkbox-bg-color-active), with theirpath:last-childset tovar(--tv-Checkbox-border-color-inverse). This update provides clearer visual feedback for active states.
168-174: Correct default fill for unchecked iconThe
.icon-checkelement now has its fill correctly set tovar(--tv-Checkbox-unchecked-border-color), and thepath:first-childis explicitly made transparent. This adjustment reduces visual ambiguity between different checkbox states.
176-178: Enhance hover feedback for checkbox iconsDuring hover, the
.icon-checknow updates its fill tovar(--tv-Checkbox-unchecked-border-color-hover), improving the user’s interaction feedback.
182-184: Proper disabled cursor stylingDisabling the input now correctly applies
cursor: not-allowed, clearly signifying the non-interactive state.
185-193: Refine disabled state icon stylingWithin the disabled state,
.icon-checked-surand.icon-halfselectnow usevar(--tv-Checkbox-checked-disabled-bg-color)and set theirpath:last-childfill tovar(--tv-Checkbox-icon-inverse-disabled). This enhances the visual distinction for disabled checkboxes.
194-200: Normalize disabled styling for unchecked iconThe
.icon-checkin a disabled state now uniformly appliesvar(--tv-Checkbox-bg-color-disabled)to both its fill andpath:first-child, ensuring consistency.packages/theme/src/option/index.less (3)
52-57: Update SVG fill for selected option iconsThe
.checked-sur.tiny-svgand.halfselect.tiny-svgselectors now apply a fill ofvar(--tv-Option-icon-color-checked)with thepath:last-childusingvar(--tv-Option-icon-inverse-disabled). This change aligns the selected state visuals with the updated theme variables.
58-67: Introduce hover state for option check iconsThe
.check.tiny-svgblock now sets a default fill usingvar(--tv-Option-icon-color-unchecked)and changes on hover tovar(--tv-Option-icon-color-hover). This provides enhanced visual cues on interaction.
113-120: Refine disabled state styling for option check iconsDisabled checkbox icons within options now correctly use
var(--tv-Option-icon-color-unchecked-disabled)for the.checkclass, with the innerpath:first-childreceivingvar(--tv-Option-bg-color-disabled). This strengthens the visual treatment for disabled options.packages/theme/src/base/dark-theme.less (2)
331-332: Adjust disabled icon opacity for dark themeThe variable
--tv-color-icon-control-disabledhas been updated torgba(255, 255, 255, 0.15)for a subtler disabled icon appearance across checkbox and radio components. Confirm that this opacity meets design requirements.
349-350: Normalize background mask variable formatChanging from
rgb(26, 26, 26, 70%)torgba(26, 26, 26, 70%)for--tv-color-bg-mask-blockensures a proper RGBA format, which is essential for transparency effects in the dark theme.packages/theme/src/button/vars.less (2)
93-95: Add loading icon color for default button stateA new variable
--tv-Button-icon-color-default-loadinghas been introduced, inheriting its value fromvar(--tv-color-icon-control, #191919). This allows for distinct styling of loading state icons for default buttons.
104-105: Introduce loading icon color for primary buttonsThe addition of
--tv-Button-icon-color-primary-loading, which draws fromvar(--tv-color-icon-inverse-tint, #ffffff), ensures that primary buttons have a dedicated loading icon color. Verify that the loading indicators integrate well with the overall theme.packages/theme/src/button/index.less (6)
5-6: Variable Declaration with Tilde
The use of the tilde (~) in the declarations of@button-prefix-clsand@svg-prefix-clsis correct. It outputs the unquoted string values and ensures proper interpolation with other Less variables.
114-115: Redundant.is-loadingRepetition
The selector repeats.is-loadingfive times:&.is-loading.is-loading.is-loading.is-loading.is-loading {This redundancy can lead to confusion and difficulty in maintenance. As noted in previous reviews, please simplify this to a single instance (
&.is-loading).
225-232: Redundant.is-disabledRepetition
The disabled state selector uses the repeated class.is-disabled(five times) as well as repeated attribute selectors. This pattern is redundant and could be streamlined to a single class to improve readability and maintenance.
248-259: Enhanced Loading State Styling
A new block for the.is-loadingstate has been added for both default and primary button styles. This addition clearly differentiates the loading state by setting SVG fill colors using the variables--tv-Button-icon-color-default-loadingand--tv-Button-icon-color-primary-loading.
Please verify that these variables are defined (e.g., invars.less) and that they provide the correct contrast for both dark and light themes.
237-245: Disabled Icon-Only Button Styling
The block for.is-only-iconunder the disabled state now updates the SVG fill and sets the background to transparent for buttons without borders. This approach is appropriate for dark mode adjustments.
Ensure thatvar(--tv-Button-icon-color-disabled)is defined with color values that maintain sufficient contrast in dark theme contexts.
373-378: Disabled Icon Hover State Styling
Within the disabled icon-only buttons, the hover state now sets the SVG fill color usingvar(--tv-Button-disabled-color-only-icon-hover). This ensures that even in the disabled state, the icon’s visual treatment is clear.
Verify that this variable is defined and that its value delivers consistent UI behavior under dark theme conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/sites/demos/pc/app/button/dynamic-disabled.spec.ts(1 hunks)examples/sites/demos/pc/app/checkbox/basic-usage.spec.ts(1 hunks)examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (2)
examples/sites/demos/pc/app/checkbox/basic-usage.spec.ts (1)
8-8:❓ Verification inconclusive
Updated SVG path selector to match updated component structure.
The selector has been changed from
.first()to.nth(1)to target the second SVG path instead of the first one. This change appears to be necessary to adapt the test to structural changes in the checkbox component's SVG implementation as part of the dark theme styling fixes.
🏁 Script executed:
#!/bin/bash # Check the structure of the SVG paths in the checkbox component to verify why we need to target the second path cd examples/sites/demos/pc rg -A 5 "checkbox__inner svg path" --type=vueLength of output: 104
Re-run verification of Vue component structure for updated SVG path selector
The previous command failed because the “vue” file type wasn’t recognized. To verify that targeting the second SVG path (using
.nth(1)) is indeed required by the updated component structure, please run the following command to search within Vue files:#!/bin/bash # Re-run search for the SVG path structure in Vue files rg --type-add "vue:*.vue" -A 5 "checkbox__inner svg path" examples/sites/demos/pcEnsure that the output confirms that the component now contains multiple SVG path elements and that the intended element is indeed the second one. This will validate the change from
.first()to.nth(1)made for the dark theme styling fixes.examples/sites/demos/pc/app/button/dynamic-disabled.spec.ts (1)
15-15:✅ Verification successful
Updated disabled button border color to match new theme specifications.
The expected border-bottom-color has been changed from
rgb(240, 240, 240)torgb(219, 219, 219), which is a slightly darker gray. This change aligns with the PR objective to fix dark theme styling issues.
🏁 Script executed:
#!/bin/bash # Check if this border color value is consistently applied in the theme CSS variables cd packages/theme/src rg -A 1 "\-\-tv\-color\-border\-disabled" --type=lessLength of output: 3539
Action: Disabled Button Border Color Update Confirmed
The test update in
examples/sites/demos/pc/app/button/dynamic-disabled.spec.tsnow correctly expects the disabled button’sborder-bottom-colorto bergb(219, 219, 219). This value matches the theme specification where the CSS variable--tv-color-border-disabledfalls back to#dbdbdb(i.e. rgb(219, 219, 219)) in several theme files (e.g.,button/vars.less). The change effectively replaces the old expectation ofrgb(240, 240, 240)to address the dark theme styling issues.
| @@ -32,5 +32,6 @@ test('下拉表格大数据', async ({ page }) => { | |||
| await expect(row).toHaveCount(6) | |||
| await row.nth(5).scrollIntoViewIfNeeded() | |||
| await expect(row).toHaveCount(6) | |||
| await page.waitForTimeout(500) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Added timeout before clicking on grid row to improve test stability.
Adding a fixed timeout of 500ms before clicking on a row element might help with test stability, but fixed timeouts can make tests flaky and unpredictable. Consider using explicit waiting conditions instead.
- await page.waitForTimeout(500)
+ // Wait for element to be stable and ready for interaction instead of using a fixed timeout
+ await page.getByRole('row', { name: '华南区 12 广东省 广州市' }).waitFor({state: 'visible', timeout: 5000})🏁 Script executed:
#!/bin/bash
# Check for other instances of waitForTimeout in the codebase that might benefit from similar improvements
cd examples/sites/demos/pc
rg "waitForTimeout" --type=tsLength of output: 25658
Replace Fixed Timeout with Explicit Wait for Element Visibility
The current fixed timeout of 500 ms may boost stability in a quick fix, but it can ultimately lead to flakiness. It’s recommended to replace this fixed delay with an explicit wait that confirms the grid row element is visible and stable before interacting with it. For instance:
- await page.waitForTimeout(500)
+ // Wait for the grid row element to become visible and stable before interaction
+ await page.getByRole('row', { name: '华南区 12 广东省 广州市' }).waitFor({ state: 'visible', timeout: 5000 })This change not only targets a more reliable condition but also aligns with improvements observed in several other tests within the codebase. Please update this instance—and consider reviewing similar waitForTimeout usages—to enhance overall test stability.
📝 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.
| await page.waitForTimeout(500) | |
| // Wait for element to be stable and ready for interaction instead of using a fixed timeout | |
| await page.getByRole('row', { name: '华南区 12 广东省 广州市' }).waitFor({ state: 'visible', timeout: 5000 }) |
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
Style
Tests