refactor(switch): remove MDC dependency#3964
refactor(switch): remove MDC dependency#3964adrianschmidt-bot wants to merge 9 commits intoLundalogik:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved the MDC dependency from the switch component: JS MDCSwitch usage and MDC-based SCSS were replaced with a custom implementation, selectors renamed from Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3964/ |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/switch/switch.scss`:
- Around line 205-215: The .switch__icon rule currently sets fill: transparent
which hides both SVG icons and prevents the opacity transitions from revealing
them; remove fill: transparent from .switch__icon and instead set fill to a
usable value (e.g., fill: currentColor or inherit) so the SVG path can be shown,
and ensure the active/inactive state selectors that animate opacity (the rules
referenced around the opacity transitions) control visibility via opacity only
while color is provided by .switch__icon; update any state-specific color if
needed so the transition reveals the icon as intended.
- Line 31: The current rule removing the native outline in switch.scss should be
preceded by a visible :focus-visible treatment so keyboard users can see focus;
add a dedicated :focus-visible selector for the switch component (e.g.,
.switch:focus-visible and the checked variant .switch--checked:focus-visible or
.switch.is-checked:focus-visible) that applies a clear visible focus style
(outline or ring/box-shadow with sufficient contrast) before the existing
outline: none rule, and apply the same :focus-visible treatment inside the
checked-state block referenced around lines 123-131 so the "on" switch shows a
distinct focus indicator even when checked.
In `@src/components/switch/switch.tsx`:
- Around line 138-140: The BEM-style class hooks ('switch--unselected',
'switch--selected', and any 'switch__*' variants) used in the Switch component
should be replaced with flat, non-BEM class names and corresponding selector
updates; update the class list generation around the switch element (where you
currently set switch: true, 'switch--unselected': !this.value,
'switch--selected': this.value) to use flattened names (e.g.,
selected/unselected or a single stateful class like selected) and then update
the matching selectors in switch.scss and all e2e selectors to the new flat
names so the component follows the repo's shadow-DOM naming convention. Ensure
you change every occurrence mentioned (including the block around lines 149-161)
so component styling and tests remain in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5b23749-8a32-4790-a995-647f8461a32a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/components/switch/switch.e2e.tsxsrc/components/switch/switch.scsssrc/components/switch/switch.tsx
src/components/switch/switch.tsx
Outdated
| switch: true, | ||
| 'switch--unselected': !this.value, | ||
| 'switch--selected': this.value, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid BEM-style hooks in the new switch DOM.
The new switch--* / switch__* names violate the shadow-DOM naming convention used in this repo. Please flatten these hooks and rename the matching selectors in switch.scss and the e2e selectors in the same pass. As per coding guidelines, "Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS."
Also applies to: 149-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/switch/switch.tsx` around lines 138 - 140, The BEM-style class
hooks ('switch--unselected', 'switch--selected', and any 'switch__*' variants)
used in the Switch component should be replaced with flat, non-BEM class names
and corresponding selector updates; update the class list generation around the
switch element (where you currently set switch: true, 'switch--unselected':
!this.value, 'switch--selected': this.value) to use flattened names (e.g.,
selected/unselected or a single stateful class like selected) and then update
the matching selectors in switch.scss and all e2e selectors to the new flat
names so the component follows the repo's shadow-DOM naming convention. Ensure
you change every occurrence mentioned (including the block around lines 149-161)
so component styling and tests remain in sync.
There was a problem hiding this comment.
Declining this one — BEM-style naming in shadow DOM is already the established pattern across the repo (e.g. button-group, chip-set, collapsible-section, etc.). Changing it here would be inconsistent with the rest of the codebase. If the repo wants to move away from BEM in shadow DOM, that'd be a separate cross-component effort.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/switch/switch.scss`:
- Around line 23-24: The new BEM-style selectors (e.g., switch__* and switch--*
introduced in switch.scss) must be renamed to flat, shadow-DOM-friendly class
names used by this repo (avoid __ and --); locate occurrences around the .switch
block and the other ranges (lines referenced in the review) and replace each
switch__foo / switch--bar with a single-level name (for example switchFoo or
switch-bar consistent with existing project naming) and update any corresponding
references in the component TSX/e2e tests to match the new flat names; ensure no
BEM-style selectors remain in switch.scss (including the sets at the other
mentioned ranges).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32ce9abd-049f-4caf-8fe4-e57931ec36b2
📒 Files selected for processing (1)
src/components/switch/switch.scss
src/components/switch/switch.scss
Outdated
| .switch { | ||
| align-items: center; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename the new selector set away from BEM.
This refactor introduces switch__* / switch--* throughout the component. In this repo, shadow-DOM component styles should use flat selectors instead, so these new hooks should be renamed before they spread further into the matching TSX/e2e changes.
As per coding guidelines, "Do not use BEM-style class names in CSS for components, as components use shadow-DOM."
Also applies to: 67-68, 87-118, 140-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/switch/switch.scss` around lines 23 - 24, The new BEM-style
selectors (e.g., switch__* and switch--* introduced in switch.scss) must be
renamed to flat, shadow-DOM-friendly class names used by this repo (avoid __ and
--); locate occurrences around the .switch block and the other ranges (lines
referenced in the review) and replace each switch__foo / switch--bar with a
single-level name (for example switchFoo or switch-bar consistent with existing
project naming) and update any corresponding references in the component TSX/e2e
tests to match the new flat names; ensure no BEM-style selectors remain in
switch.scss (including the sets at the other mentioned ranges).
There was a problem hiding this comment.
Same as the TSX comment — BEM-style naming in shadow DOM is the established pattern across the repo. Declining this one for consistency.
There was a problem hiding this comment.
This was already considered and declined in the previous review round. The BEM-style naming (switch__* / switch--*) is consistent with how other components in this repo structure their shadow-DOM styles. No changes needed here.
src/components/switch/switch.tsx
Outdated
| <div class="switch__handle"> | ||
| <div class="switch__shadow" /> | ||
| <div class="switch__icons"> | ||
| <svg |
There was a problem hiding this comment.
@adrianschmidt-bot In the previous design, we don't display the SVGs that illustrate a checkmark (when checked/selected) and a horizontal line (when unchecked) when the switch is enabled. They only show up when the switch is disabled since everything would be gray, and a user might have a hard time understanding if the switch is visualizing ON or OFF mode.
In general, when refactoring, do not replicate the MDC's internal structure. It seems you are keeping the same HTML elements, and just renaming them, and try to recreate the same styles as MDC had. There are way better ways of creating a switch, that visually is appealing, but without any of these structural complications.
Remove as much as you can. Make a fresh minimal switch with least amount of code. You don't need to stay loyal to what MDC had.
After that, I'll take over and review the styles. I need to also make sure the switch works and aligns well in limel-form and need to check a few other details, which are hard to explain here.
Let me know when refactoring is done, and I'll review and tweak.
There was a problem hiding this comment.
Good call — you're right, I was just renaming MDC's structure without questioning it.
Rewrote it from scratch. The switch is now:
- Track (
<span>) + handle (<span>) — that's it - SVG icons only render when
disabled(checkmark/dash to clarify on/off in grayscale) - No shadow wrapper, no handle-track, no icon container — just the two elements needed
- SCSS went from ~200 lines to ~80
⚡ Fixup: 232b63a
Ready for your style review whenever you want to take over.
adrianschmidt-bot
force-pushed
the
refactor/remove-mdc-switch
branch
from
9bb5829 to
232b63a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/switch/switch.tsx (2)
134-161: 🛠️ Refactor suggestion | 🟠 MajorWrap JSX elements in
<Host>instead of array literal.Same issue here—the non-readonly render path also returns an array literal. As per coding guidelines, "When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element."♻️ Proposed fix
- return [ + return ( + <Host> <button id={this.fieldId} class={{ switch: true, 'switch--selected': this.value, }} type="button" role="switch" aria-checked={this.value} disabled={this.disabled} onClick={this.handleClick} aria-controls={this.helperText ? this.helperTextId : undefined} > <span class="switch__track"> <span class="switch__handle"> {this.disabled && this.renderStateIcon()} </span> </span> - </button>, + </button> <label class={`${this.disabled ? 'disabled' : ''}`} htmlFor={this.fieldId} > {this.label} - </label>, + </label> {this.renderHelperLine()} - ]; + </Host> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/switch/switch.tsx` around lines 134 - 161, The render method currently returns an array literal of JSX elements (the button using this.fieldId/this.value/this.disabled and onClick={this.handleClick}, the label using this.label and htmlFor={this.fieldId}, and this.renderHelperLine()), which violates the guideline; change the return to wrap these elements inside a single <Host> element instead of an array literal, preserving all props/attributes, children (including the nested <span class="switch__track">, this.renderStateIcon() call), and the order of elements so behavior of methods like renderHelperLine() and handleClick remains identical.
121-131: 🛠️ Refactor suggestion | 🟠 MajorWrap JSX elements in
<Host>instead of array literal.The render method returns an array literal here. Per coding guidelines, multiple JSX elements should be wrapped in StencilJS's special
<Host>element instead.♻️ Proposed fix
- return [ - <limel-dynamic-label + return ( + <Host> + <limel-dynamic-label value={this.value} aria-controls={ this.helperText ? this.helperTextId : undefined } defaultLabel={{ text: this.label, icon: icon }} labels={this.readonlyLabels} - />, - this.renderHelperLine(), - ]; + /> + {this.renderHelperLine()} + </Host> + );You'll also need to import
Hostfrom@stencil/core:import { Component, Element, Event, EventEmitter, h, + Host, Prop, State, } from '@stencil/core';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/switch/switch.tsx` around lines 121 - 131, The render method currently returns an array of JSX nodes; update the Switch component's render() to wrap the returned elements in a Stencil Host element instead of an array literal: import Host from '@stencil/core' and return <Host> containing the <limel-dynamic-label ... /> and the result of this.renderHelperLine(); ensure props used (this.value, this.helperTextId, this.label, icon, this.readonlyLabels) remain unchanged and that this.renderHelperLine() is still called inside the Host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/switch/switch.tsx`:
- Around line 134-161: The render method currently returns an array literal of
JSX elements (the button using this.fieldId/this.value/this.disabled and
onClick={this.handleClick}, the label using this.label and
htmlFor={this.fieldId}, and this.renderHelperLine()), which violates the
guideline; change the return to wrap these elements inside a single <Host>
element instead of an array literal, preserving all props/attributes, children
(including the nested <span class="switch__track">, this.renderStateIcon()
call), and the order of elements so behavior of methods like renderHelperLine()
and handleClick remains identical.
- Around line 121-131: The render method currently returns an array of JSX
nodes; update the Switch component's render() to wrap the returned elements in a
Stencil Host element instead of an array literal: import Host from
'@stencil/core' and return <Host> containing the <limel-dynamic-label ... /> and
the result of this.renderHelperLine(); ensure props used (this.value,
this.helperTextId, this.label, icon, this.readonlyLabels) remain unchanged and
that this.renderHelperLine() is still called inside the Host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 425caedb-8138-4d29-8c41-a908d61cec73
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/components/switch/switch.e2e.tsxsrc/components/switch/switch.scsssrc/components/switch/switch.tsx
|
Declining the |
@adrianschmidt-bot Actually, we want to move away from the array literal return pattern, and this is a great opportunity to make sure this component uses the pattern we prefer. |
|
You're right — updated! Wrapped both render paths in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/switch/switch.tsx`:
- Around line 160-165: The label element uses a template literal for its class;
update it to use the object/class-map syntax consistent with the rest of the
component (replace class={`${this.disabled ? 'disabled' : ''}`} with an
object-style class binding such as class={{ disabled: this.disabled }}), keeping
htmlFor={this.fieldId} and the label content unchanged so the disabled state is
applied the same way as other elements in this component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce7a30b1-13f3-4ca6-a96b-63c0bd77487f
📒 Files selected for processing (1)
src/components/switch/switch.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/switch/switch.tsx (1)
141-155: 🛠️ Refactor suggestion | 🟠 MajorFlatten the new shadow-DOM class hooks.
switch--selectedand theswitch__*names reintroduce BEM-style hooks. Please rename them here and update the matching selectors insrc/components/switch/switch.scssandsrc/components/switch/switch.e2e.tsxin the same pass.As per coding guidelines, "Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS."
Also applies to: 174-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/switch/switch.tsx` around lines 141 - 155, Rename the BEM-style classes used in the Switch component to flattened shadow-DOM hooks and update matching selectors in the stylesheet and tests: change "switch--selected" to "selected", "switch__track" to "track", and "switch__handle" to "handle" in the JSX (including the additional occurrences around lines 174-181), then update src/components/switch/switch.scss and src/components/switch/switch.e2e.tsx to use .selected, .track, and .handle selectors instead of the old BEM names so all references remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/switch/switch.tsx`:
- Around line 146-152: The switch currently uses aria-controls to reference the
helper text, which doesn't make that text part of the switch's accessible
description; update the JSX in the switch component (the element with
role="switch" and props aria-checked, disabled, onClick) to use aria-describedby
instead of aria-controls when this.helperText exists (use this.helperTextId as
the referenced id), i.e. replace the aria-controls conditional with
aria-describedby={this.helperText ? this.helperTextId : undefined} so assistive
tech will read the helper/invalid text on focus.
---
Duplicate comments:
In `@src/components/switch/switch.tsx`:
- Around line 141-155: Rename the BEM-style classes used in the Switch component
to flattened shadow-DOM hooks and update matching selectors in the stylesheet
and tests: change "switch--selected" to "selected", "switch__track" to "track",
and "switch__handle" to "handle" in the JSX (including the additional
occurrences around lines 174-181), then update src/components/switch/switch.scss
and src/components/switch/switch.e2e.tsx to use .selected, .track, and .handle
selectors instead of the old BEM names so all references remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb0f8099-eb8e-4873-9bfb-e6710ac5c58e
📒 Files selected for processing (1)
src/components/switch/switch.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/switch/switch.tsx (1)
141-156: 🛠️ Refactor suggestion | 🟠 MajorFlatten the remaining switch hook names.
switch--selected,switch__track,switch__handle, andswitch__iconare still BEM-style hooks in a shadow-DOM component. Please rename them together with the matching selectors insrc/components/switch/switch.scssso this refactor follows the repo convention.As per coding guidelines, "Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS."
Also applies to: 174-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/switch/switch.tsx` around lines 141 - 156, Rename the BEM-style class hooks used in the Switch component to flattened names and update the SCSS selectors to match: replace "switch--selected", "switch__track", "switch__handle", and "switch__icon" in the JSX/class map (the element using class={{ switch: true, 'switch--selected': this.value }} and the inner <span class="switch__track"> / <span class="switch__handle"> and any class added inside renderStateIcon()) with flattened equivalents (e.g., selected, track, handle, icon) and update the corresponding selectors in switch.scss; also update any other references (tests, utilities, or other components) that reference those class names so the component still compiles and renderStateIcon continues to add the correct class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/switch/switch.tsx`:
- Around line 174-183: The inline SVG icons for the switch (the elements with
class "switch__icon") are currently exposed to the accessibility tree and should
be hidden because the component already exposes state via role="switch" and
aria-checked; update both SVG elements returned in
src/components/switch/switch.tsx (the SVG with path "M19.69,5.23..." and the one
with path "M20 13H4v-2h16v2z") to include aria-hidden="true" (and optionally
focusable="false" for IE/SVG keyboard focus) so assistive tech ignores these
decorative graphics.
---
Duplicate comments:
In `@src/components/switch/switch.tsx`:
- Around line 141-156: Rename the BEM-style class hooks used in the Switch
component to flattened names and update the SCSS selectors to match: replace
"switch--selected", "switch__track", "switch__handle", and "switch__icon" in the
JSX/class map (the element using class={{ switch: true, 'switch--selected':
this.value }} and the inner <span class="switch__track"> / <span
class="switch__handle"> and any class added inside renderStateIcon()) with
flattened equivalents (e.g., selected, track, handle, icon) and update the
corresponding selectors in switch.scss; also update any other references (tests,
utilities, or other components) that reference those class names so the
component still compiles and renderStateIcon continues to add the correct class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c66ea62-5ebd-44b3-9558-ab62bb9d4bec
📒 Files selected for processing (1)
src/components/switch/switch.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/components/switch/switch.tsx (2)
141-156: 🛠️ Refactor suggestion | 🟠 MajorFlatten the new switch hooks before they spread further.
switch--selectedand theswitch__*classes reintroduce BEM naming into a shadow-DOM component. Please rename these hooks together with the matching SCSS and e2e selectors in the same pass.As per coding guidelines, "Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS."
Also applies to: 175-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/switch/switch.tsx` around lines 141 - 156, The component introduces BEM-style class names (e.g., "switch--selected", "switch__track", "switch__handle") inside the shadow-DOM; replace these with flat, non-BEM class names and update all corresponding usages in the same commit: change the class map in the host element (where class={{ switch: true, 'switch--selected': this.value }} is set) and the inner spans that use "switch__track" and "switch__handle" to the new flat names, then update the SCSS selectors and any e2e test selectors that reference those BEM names; ensure the behavior tied to this.value, this.disabled, renderStateIcon(), handleClick and helperTextId remains unchanged while renaming.
126-128:⚠️ Potential issue | 🟡 MinorUse the descriptive ARIA relation in the readonly branch too.
Line 126 still wires helper text through
aria-controls, so assistive tech will not treat the helper line as descriptive text the way the interactive branch now does.♿ Suggested fix
- aria-controls={ + aria-describedby={ this.helperText ? this.helperTextId : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/switch/switch.tsx` around lines 126 - 128, The readonly branch of the Switch component still sets aria-controls to this.helperTextId, preventing assistive tech from treating helper text as descriptive—update the readonly rendering path in the Switch component (where aria-controls is set) to use the same descriptive ARIA relation as the interactive branch: replace aria-controls with aria-describedby (or include aria-describedby alongside aria-controls) referencing this.helperTextId (and ensure this.helperTextId is applied only when this.helperText exists) so helper text is exposed as descriptive text to AT users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/switch/switch.tsx`:
- Around line 141-156: The component introduces BEM-style class names (e.g.,
"switch--selected", "switch__track", "switch__handle") inside the shadow-DOM;
replace these with flat, non-BEM class names and update all corresponding usages
in the same commit: change the class map in the host element (where class={{
switch: true, 'switch--selected': this.value }} is set) and the inner spans that
use "switch__track" and "switch__handle" to the new flat names, then update the
SCSS selectors and any e2e test selectors that reference those BEM names; ensure
the behavior tied to this.value, this.disabled, renderStateIcon(), handleClick
and helperTextId remains unchanged while renaming.
- Around line 126-128: The readonly branch of the Switch component still sets
aria-controls to this.helperTextId, preventing assistive tech from treating
helper text as descriptive—update the readonly rendering path in the Switch
component (where aria-controls is set) to use the same descriptive ARIA relation
as the interactive branch: replace aria-controls with aria-describedby (or
include aria-describedby alongside aria-controls) referencing this.helperTextId
(and ensure this.helperTextId is applied only when this.helperText exists) so
helper text is exposed as descriptive text to AT users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c642050-6b34-4823-bd5e-9b6ccec486c9
📒 Files selected for processing (1)
src/components/switch/switch.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Remove @material/switch JS and SCSS imports. Replace with custom CSS implementation that maintains the same visual appearance and behavior. The switch now uses pure CSS for track, handle, and state transitions without any MDC runtime. fix: Lundalogik#3947
fixup! refactor(switch): remove MDC dependency - Fix fill: transparent hiding SVG icons (use contrast-1100) - Add :focus-visible style for keyboard accessibility
adrianschmidt
force-pushed
the
refactor/remove-mdc-switch
branch
from
f05aacd to
329b0ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/switch/switch.tsx (1)
141-148: 🧹 Nitpick | 🔵 TrivialUse
aria-checkedas the single selected-state hook.
aria-checkedalready exposes the same state on this button, soswitch--selectedgives SCSS and tests a second state surface to keep in sync. Styling from[aria-checked='true']would make this refactor smaller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/switch/switch.tsx` around lines 141 - 148, Remove the duplicated selected-state class and rely solely on aria-checked as the single source of truth: remove the conditional 'switch--selected' from the class prop on the button (the element with role="switch" and attribute aria-checked={this.value}), update the component's class rendering to only emit the base "switch" class, and migrate any SCSS selectors and tests that target .switch--selected to instead target [aria-checked='true'] (and [aria-checked='false'] where appropriate) so styling and tests use the aria attribute as the single selected-state hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/switch/switch.scss`:
- Around line 74-80: The focus-visible shadow is being overridden by the hover
rule; update the CSS so focus wins when both states apply by either moving the
.switch:focus-visible .switch__handle rule after the hover rule or adding a more
specific selector that preserves the focus shadow during hover (e.g. add a
.switch:focus-visible:hover .switch__handle rule that sets box-shadow:
var(--shadow-depth-8-focused)); target the existing selectors
(.switch:focus-visible .switch__handle and .switch:hover:not(:disabled)
.switch__handle) when making this change.
- Around line 43-53: The switch styles currently hard-code the new tokens and
drop the legacy MDC variables; update the CSS to provide a compatibility layer
by referencing the old `--mdc-switch-*` hooks as fallbacks or aliasing them into
the new tokens so existing theme overrides still apply. Specifically, modify the
rules for the selector `.switch--selected .switch__track` and the other affected
selectors (lines mentioned: 62-63, 87-93) to use `var(--mdc-switch-...,
var(--lime-primary-color, var(--limel-theme-primary-color)))` (and analogous
mappings for other color usages) or alternatively define the new
`--lime-primary-color`/other new tokens by aliasing them to the legacy
`--mdc-switch-*` variables with appropriate fallbacks.
---
Duplicate comments:
In `@src/components/switch/switch.tsx`:
- Around line 141-148: Remove the duplicated selected-state class and rely
solely on aria-checked as the single source of truth: remove the conditional
'switch--selected' from the class prop on the button (the element with
role="switch" and attribute aria-checked={this.value}), update the component's
class rendering to only emit the base "switch" class, and migrate any SCSS
selectors and tests that target .switch--selected to instead target
[aria-checked='true'] (and [aria-checked='false'] where appropriate) so styling
and tests use the aria attribute as the single selected-state hook.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4eefde8-4eae-4ced-a806-ea2e039b1724
📒 Files selected for processing (3)
src/components/switch/switch.e2e.tsxsrc/components/switch/switch.scsssrc/components/switch/switch.tsx
| background-color: rgb(var(--contrast-700)); | ||
| transition: background-color $animation-duration ease; | ||
| padding: 0; | ||
| } | ||
|
|
||
| --mdc-switch-unselected-track-color: rgb(var(--contrast-700)); | ||
| --mdc-switch-unselected-focus-track-color: rgb(var(--contrast-800)); | ||
| --mdc-switch-unselected-pressed-track-color: rgb(var(--contrast-800)); | ||
| --mdc-switch-unselected-hover-track-color: rgb(var(--contrast-800)); | ||
| --mdc-switch-selected-focus-track-color: var( | ||
| --lime-primary-color, | ||
| var(--limel-theme-primary-color) | ||
| ); | ||
| --mdc-switch-selected-pressed-track-color: var( | ||
| --lime-primary-color, | ||
| var(--limel-theme-primary-color) | ||
| ); | ||
| --mdc-switch-selected-track-color: var( | ||
| --lime-primary-color, | ||
| var(--limel-theme-primary-color) | ||
| ); | ||
| --mdc-switch-selected-hover-track-color: var( | ||
| .switch--selected .switch__track { | ||
| background-color: var( | ||
| --lime-primary-color, | ||
| var(--limel-theme-primary-color) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add a compatibility layer for the removed switch color hooks.
These declarations now hard-wire the switch colors to the new tokens only. I don't see any aliasing for the old --mdc-switch-* hooks in the same PR, so existing theme overrides will silently stop affecting limel-switch after upgrade. Please either map the legacy variables onto these rules or update/document every affected usage as a breaking change. Based on learnings, "In lime-elements code reviews, flag any removal of CSS custom properties (including those with var(--property-name, fallback) syntax) that doesn't include corresponding updates to all usage instances or backwards compatibility implementation in the same PR."
Also applies to: 62-63, 87-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/switch/switch.scss` around lines 43 - 53, The switch styles
currently hard-code the new tokens and drop the legacy MDC variables; update the
CSS to provide a compatibility layer by referencing the old `--mdc-switch-*`
hooks as fallbacks or aliasing them into the new tokens so existing theme
overrides still apply. Specifically, modify the rules for the selector
`.switch--selected .switch__track` and the other affected selectors (lines
mentioned: 62-63, 87-93) to use `var(--mdc-switch-..., var(--lime-primary-color,
var(--limel-theme-primary-color)))` (and analogous mappings for other color
usages) or alternatively define the new `--lime-primary-color`/other new tokens
by aliasing them to the legacy `--mdc-switch-*` variables with appropriate
fallbacks.
There was a problem hiding this comment.
|
Looks good to me, but I've only checked the docs page for the switch itself, and I'm on my phone so I haven't checked stuff like focus and hover. @Kiarokh I think this one is ready for your discerning eye 🙂 |
3 participants
Remove
@material/switchJS and SCSS imports. Replace with custom CSS implementation that maintains the same visual appearance and behavior.The switch now uses pure CSS for track, handle, and state transitions without any MDC runtime.
Changes
MDCSwitchclass instantiation and lifecycle management@Watch('value')handler (no longer needed without MDC).mdc-switch*CSS classes with.switch*equivalentsTesting
Closes #3947
Summary by CodeRabbit