fix(switch): modify the saas theme style #3773
Conversation
WalkthroughRestructures Switch styling by replacing pseudo-element positioning with a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential points of attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/theme-saas/src/switch/index.less (1)
72-74: Consider making text width configurable.The
&__textelement has a fixed width of 46px. Consider whether this should be configurable or responsive to accommodate different text lengths or internationalization needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/theme-saas/src/svgs/loading-shadow.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
packages/theme-saas/src/switch/index.less(2 hunks)packages/vue/src/switch/src/index.ts(1 hunks)
🔇 Additional comments (3)
packages/theme-saas/src/switch/index.less (3)
30-49: Well-structured button element implementation.The new
&__buttonelement structure with flex centering, smooth transitions, and proper positioning logic is a solid improvement over the pseudo-element approach. The use ofcalc(100% - 18px)for checked state positioning correctly accounts for the button size and padding.
1-86: Request visual regression testing.This PR significantly restructures the switch component's visual implementation, changing from a pseudo-element to a button-based approach, updating dimensions, and introducing new loading states. Visual regression testing would help ensure no unintended visual changes or broken states.
Consider running visual regression tests (e.g., with Percy, Chromatic, or Playwright screenshots) to verify:
- Switch appearance in all states (default, checked, disabled, loading)
- Text visibility with
showTextenabled/disabled- Mini variant sizing
- Transitions and animations
- Cross-browser rendering consistency
7-7: The review comment is based on incorrect information about the changes.No changes exist in this file. The git diff is empty, and the working tree is clean. The switch width has always been
w-10(40px) in both the current and previous versions—there is no change from 50px. The file being reviewed (packages/theme-saas/src/switch/index.lessat line 7) shows@apply ~'w-10';in both states, confirming no modification occurred in this PR.The review comment's assertion that "the switch width changed from an implied 50px to
w-10(40px)" does not reflect the actual changes made.Likely an incorrect or invalid review comment.
| &__on-loading { | ||
| @apply text-xs; | ||
| fill: #b3d6ff; | ||
| } | ||
|
|
||
| &__off-loading { | ||
| @apply text-xs; | ||
| fill: #dbdbdb; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace hardcoded colors with design tokens.
The loading states use hardcoded color values (#b3d6ff, #dbdbdb) for the fill property. These should be replaced with design tokens for consistency.
&__on-loading {
@apply text-xs;
- fill: #b3d6ff;
+ fill: var(--tv-color-brand-light);
}
&__off-loading {
@apply text-xs;
- fill: #dbdbdb;
+ fill: var(--tv-color-bg-disabled);
}Note: Adjust the token names to match your actual design system tokens. The example uses common naming patterns, but you should verify the correct token names in your theme configuration.
📝 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.
| &__on-loading { | |
| @apply text-xs; | |
| fill: #b3d6ff; | |
| } | |
| &__off-loading { | |
| @apply text-xs; | |
| fill: #dbdbdb; | |
| } | |
| &__on-loading { | |
| @apply text-xs; | |
| fill: var(--tv-color-brand-light); | |
| } | |
| &__off-loading { | |
| @apply text-xs; | |
| fill: var(--tv-color-bg-disabled); | |
| } |
🤖 Prompt for AI Agents
In packages/theme-saas/src/switch/index.less around lines 62 to 70, the
.__on-loading and .__off-loading rules use hardcoded hex colors for fill
(#b3d6ff and #dbdbdb); replace those hex values with the appropriate design
tokens from our theme (for example use something like @color-blue-200 or
@token-fill-on-loading for the on state and @color-gray-300 or
@token-fill-off-loading for the off state), update the imports or variables if
needed to reference the theme token definitions, and ensure the final LESS
compiles and the token names match the project's design token keys.
| type: Boolean || undefined, | ||
| default: undefined | ||
| default: false |
There was a problem hiding this comment.
Fix incorrect prop type syntax.
The type declaration Boolean || undefined is syntactically incorrect for Vue props. The || operator evaluates to the Boolean constructor at runtime, not a union type. For optional Boolean props, use [Boolean] or just Boolean.
Apply this diff:
showText: {
- type: Boolean || undefined,
+ type: Boolean,
default: false
},📝 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.
| type: Boolean || undefined, | |
| default: undefined | |
| default: false | |
| type: Boolean, | |
| default: false |
🤖 Prompt for AI Agents
In packages/vue/src/switch/src/index.ts around lines 35 to 36, the prop type is
declared as `Boolean || undefined` which is invalid; replace it with a proper
Vue prop type for optional booleans such as `Boolean` or `[Boolean]`, and keep
the default as `false`. Update the prop definition to use the correct type array
or single constructor so Vue recognizes it as an optional Boolean prop and
remove the `|| undefined` expression.
| showText: { | ||
| type: Boolean || undefined, | ||
| default: undefined | ||
| default: false |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the default value change doesn't break existing usage.
Changing the default from undefined to false may alter component behavior for consumers who rely on the implicit undefined default. Ensure that all existing usages of the Switch component have been tested with this change.
Run the following script to identify all Switch component usages:
🏁 Script executed:
#!/bin/bash
# Description: Find all Switch component usages to verify impact of showText default change
# Search for Switch component imports and usage patterns
rg -n --type vue --type ts --type tsx -C3 '\bSwitch\b' -g '!node_modules' -g '!dist' -g '!packages/vue/src/switch'
# Also search for showText prop usage
rg -n --type vue --type ts --type tsx -C3 'showText' -g '!node_modules' -g '!dist' -g '!packages/vue/src/switch'Length of output: 55
I need to regenerate the search script with corrected ripgrep syntax since Vue file types aren't recognized natively. Let me search more broadly:
🏁 Script executed:
#!/bin/bash
# Description: Find all Switch component usages - fixed search without vue file type
# Search for Switch imports and usage in all files
rg -n 'Switch' -C3 -g '!node_modules' -g '!dist' -g '!packages/vue/src/switch/src/index.ts'
# Search for showText prop usage in all files
rg -n 'showText' -C3 -g '!node_modules' -g '!dist' -g '!packages/vue/src/switch'Length of output: 188649
Let me examine the actual Switch component definition to verify the current state of the props:The web search didn't return the specific file. Let me directly inspect the Switch component index file:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the Switch component prop definition around line 36
cat packages/vue/src/switch/src/index.ts | head -50 | tail -25
Length of output: 396
Fix default value to preserve designConfig behavior
The current default: false breaks theme-aware behavior. The renderless logic checks if (props.showText === undefined) to use designConfig fallback (Aurora/Saas both set showText: true), but with default: false, this condition never triggers. Components without explicit showText prop will get false instead of the theme's intended default.
Change the default to undefined to restore the designConfig fallback mechanism:
showText: {
type: [Boolean, undefined],
default: undefined
}This ensures Aurora and Saas themes display text by default while allowing explicit false to disable it.
🤖 Prompt for AI Agents
In packages/vue/src/switch/src/index.ts around line 36, the prop definition sets
showText default to false which prevents the renderless logic from falling back
to designConfig; change the showText prop type to allow undefined and set its
default to undefined so the component can detect when showText is not provided
and use the theme (designConfig) fallback; update the prop signature accordingly
(type includes Boolean and undefined) and set default to undefined so explicit
false still works but absent prop defers to designConfig.
James-9696
changed the title
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 (1)
packages/theme-saas/src/switch/index.less(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (3)
packages/theme-saas/src/switch/index.less (3)
41-41: Clarify thecontentproperty syntax.Line 41 uses
@apply content-[''];, which mixes Tailwind's arbitrary value syntax with the content property. Verify this compiles and produces the intended behavior (empty pseudo-element content).If this syntax is not standard for your build tooling, consider using explicit LESS:
- @apply content-['']; + content: '';
30-74: Address concerns about extensive modifications to a heavily-used component.A previous reviewer noted (in Chinese) that this is a commonly-used component and questioned why so many style modifications are needed. If there were design issues, they should have been reported. The restructuring from pseudo-elements to a new
__buttonelement is significant and could impact many downstream consumers.Please provide clarity on:
- What specific visual or functional issues this change addresses.
- Whether this has been tested with existing switch consumers to rule out regressions.
- Why the switch from a pseudo-element-based layout to a DOM-based
__buttonelement was necessary.This will help ensure the changes are intentional and well-justified.
30-49: Vue template structure properly updated to align with new__buttonelement.Verification confirms all required changes are in place:
- ✓ Vue template renders new elements:
<span class="tiny-switch__button">(line 27),tiny-switch__on-loadingandtiny-switch__off-loadingloading states, andtiny-switch__textfor text display.- ✓ LESS file properly updated with explicit
&__buttonstyling (lines 34–49) and checked state positioning logic (&&-checked &__button, line 30). No pseudo-element remnants found.- ✓ No conflicting styles in related LESS files or theme variables that could cause regressions.
The migration from pseudo-element-based positioning to an explicit element structure is complete and internally consistent.
| &&-checked.disabled, | ||
| &.disabled { | ||
| @apply cursor-not-allowed; | ||
| background-color: #dbdbdb; | ||
| border-color: #dbdbdb; | ||
| } | ||
|
|
||
| &&-checked.disabled { | ||
| @apply bg-color-brand-disabled; | ||
| } |
There was a problem hiding this comment.
Replace hardcoded colors with design tokens (recurring issue).
Lines 54–55 (disabled states) and lines 64, 69 (loading states) still use hardcoded hex values (#dbdbdb, #b3d6ff) instead of design tokens. This was flagged in the previous review and remains unresolved. Design tokens ensure theming consistency and maintainability.
Apply this diff to use design tokens:
&&-checked.disabled,
&.disabled {
@apply cursor-not-allowed;
- background-color: #dbdbdb;
- border-color: #dbdbdb;
+ @apply bg-color-bg-disabled;
+ @apply border-color-border-disabled;
}
&&-checked.disabled {
- @apply bg-color-brand-disabled;
+ @apply bg-color-bg-disabled;
}
&__on-loading {
@apply text-xs;
- fill: #b3d6ff;
+ fill: var(--tv-color-brand-light);
}
&__off-loading {
@apply text-xs;
- fill: #dbdbdb;
+ fill: var(--tv-color-bg-disabled);
}Verify that these design tokens (bg-color-bg-disabled, border-color-border-disabled, --tv-color-brand-light, --tv-color-bg-disabled) exist in your design system.
Also applies to: 62-70
🤖 Prompt for AI Agents
In packages/theme-saas/src/switch/index.less around lines 51–60 (and related
disabled/loading states in 62–70), replace hardcoded hex colors (#dbdbdb,
#b3d6ff) with the design tokens: use bg-color-bg-disabled for background,
border-color-border-disabled for border, and the brand/loading token variables
(--tv-color-brand-light and --tv-color-bg-disabled) where the loading/brand
light color is required; ensure you use the proper token syntax used in the
project (CSS variable or Less variable) and confirm those tokens exist in the
design system before committing.
2 participants
PR
修改前：
修改后：
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
修改switch组件saas主题
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