fix: add desaturated variant border in readonly checked state #4
Conversation
Only applies when a color variant (primary, success, warning, error, contrast) is set. Toggles without a variant keep the neutral readonly border. Close #2
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR updates the ChangesReadonly State and Theme Variant Styling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/resources/META-INF/resources/frontend/fc-toggle-button.js`:
- Around line 354-356: The readonly slider rule currently hardcodes "top: 2px"
in the :host([readonly]) .slider selector which overrides size-specific offsets
and misaligns non-default sizes; change this to either remove the fixed top and
rely on the existing size-specific selectors (e.g.,
:host([size="small"][readonly]) .slider, :host([size="large"][readonly])
.slider, :host([size="longswipe"][readonly]) .slider) or replace the hardcoded
value with a CSS custom property (e.g., top: var(--fc-toggle-slider-top)) and
ensure each size variant sets that custom property so readonly state preserves
per-size offsets in the slider styles.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02506253-03b5-45dc-887f-bd9ec629a2bf
📒 Files selected for processing (1)
src/main/resources/META-INF/resources/frontend/fc-toggle-button.js
| :host([readonly]) .slider { | ||
| top: 1px; | ||
| top: 2px; | ||
| left: 1px; |
There was a problem hiding this comment.
Readonly slider vertical offset now regresses size variants.
Line 355 hardcodes top: 2px for all readonly toggles, overriding small/large/longswipe size-specific offsets and causing misalignment outside the default size.
Suggested fix
:host([readonly]) .slider {
top: 2px;
left: 1px;
box-shadow: none;
background-color: var(--lumo-contrast-40pct, var(--vaadin-background-container-strong));
}
+
+:host([readonly][theme~="small"]) .slider,
+:host([readonly][theme~="longswipe"][theme~="small"]) .slider {
+ top: 1px;
+}
+
+:host([readonly][theme~="large"]) .slider,
+:host([readonly][theme~="longswipe"][theme~="large"]) .slider {
+ top: 3px;
+}📝 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.
| :host([readonly]) .slider { | |
| top: 1px; | |
| top: 2px; | |
| left: 1px; | |
| :host([readonly]) .slider { | |
| top: 2px; | |
| left: 1px; | |
| box-shadow: none; | |
| background-color: var(--lumo-contrast-40pct, var(--vaadin-background-container-strong)); | |
| } | |
| :host([readonly][theme~="small"]) .slider, | |
| :host([readonly][theme~="longswipe"][theme~="small"]) .slider { | |
| top: 1px; | |
| } | |
| :host([readonly][theme~="large"]) .slider, | |
| :host([readonly][theme~="longswipe"][theme~="large"]) .slider { | |
| top: 3px; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/META-INF/resources/frontend/fc-toggle-button.js` around
lines 354 - 356, The readonly slider rule currently hardcodes "top: 2px" in the
:host([readonly]) .slider selector which overrides size-specific offsets and
misaligns non-default sizes; change this to either remove the fixed top and rely
on the existing size-specific selectors (e.g., :host([size="small"][readonly])
.slider, :host([size="large"][readonly]) .slider,
:host([size="longswipe"][readonly]) .slider) or replace the hardcoded value with
a CSS custom property (e.g., top: var(--fc-toggle-slider-top)) and ensure each
size variant sets that custom property so readonly state preserves per-size
offsets in the slider styles.
There was a problem hiding this comment.
The harcording was already there, only the value was updated. But I did some testing and the value makes the readonly toggle not looked align in combination with those other variants. So the best thing is to handle this in a different issue and pull request. I will revert this change for now. Thnaks for making me look!
There was a problem hiding this comment.
@paodb, that sounds like a solid plan — reverting keeps this PR focused and avoids introducing a new regression while fixing the original issue. Glad the review was helpful!
Would you like me to open a GitHub issue to track the slider alignment fix for size variants (small, large, longswipe) in the readonly state, so it's not lost? I can create it with the relevant context from this discussion.
There was a problem hiding this comment.
Yes please, create the issue, I will update it later. Don't assign the issue to anyone.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo FlowingCode/ToggleButton \
--title "fix: readonly slider vertical alignment regression for size variants (small, large, longswipe)" \
--body "## Summary
The \`:host([readonly]) .slider\` rule hardcodes \`top: 2px\`, which overrides the size-specific \`top\` offsets defined for the \`small\`, \`large\`, and \`longswipe\` theme variants. This causes the slider knob to be vertically misaligned when a size variant is combined with the readonly state.
## Expected Behavior
The slider knob should remain vertically centered within the switch track for all size variants in readonly state, consistent with their non-readonly appearance.
## Current Behavior
- **Default / medium**: \`top: 2px\` ✅ (correct)
- **Small**: should be \`top: 1px\`, but \`top: 2px\` is applied ❌
- **Large**: should be \`top: 3px\`, but \`top: 2px\` is applied ❌
- **Longswipe + small**: should be \`top: 1px\`, but \`top: 2px\` is applied ❌
- **Longswipe + large**: should be \`top: 3px\`, but \`top: 2px\` is applied ❌
## Proposed Fix
Add per-size override rules under the readonly selector, for example:
\`\`\`css
:host([readonly][theme~=\"small\"]) .slider,
:host([readonly][theme~=\"longswipe\"][theme~=\"small\"]) .slider {
top: 1px;
}
:host([readonly][theme~=\"large\"]) .slider,
:host([readonly][theme~=\"longswipe\"][theme~=\"large\"]) .slider {
top: 3px;
}
\`\`\`
Alternatively, a CSS custom property (e.g. \`--fc-toggle-slider-top\`) could be introduced so each size variant sets the property and the readonly rule inherits it automatically.
## Context
Identified during review of PR `#4` (https://github.com/FlowingCode/ToggleButton/pull/4) — specifically the discussion at https://github.com/FlowingCode/ToggleButton/pull/4#discussion_r3229628971.
The fix for the original read-only alignment issue (PR `#4`) was reverted to avoid introducing this regression. This issue tracks the follow-up work.
Reported by `@paodb`."
Length of output: 53
@paodb, the issue has been created: FlowingCode/ToggleButton#5 — fix: readonly slider vertical alignment regression for size variants (small, large, longswipe).
It includes a summary of the problem, the expected vs. current top values for each size variant, and the proposed fix with override selectors. Feel free to update it as needed!
There was a problem hiding this comment.
Yes but you mention that the value is 2px and I reverted that. the problem is not that is 2px, is that for readonly should be 2px but when combined with the other variants, some combinations don't look aligned. Can you check?
There was a problem hiding this comment.
🧩 Analysis chain
Let me pull up the current issue and verify the math, then update it if the framing needs correcting.
🏁 Script executed:
#!/bin/bash
gh issue view 5 --repo FlowingCode/ToggleButton --json title,body,number
Length of output: 1906
You're right — the issue framing needs correcting. Since you reverted to top: 1px, I need to recalculate which sizes are actually broken now.
With a 1px dashed border and box-sizing: border-box, the inner track height shrinks by 2px (1px top + 1px bottom). Here's the correct math:
| Size | Switch H | Slider H | Inner H | Correct top |
Current (top: 1px) |
|---|---|---|---|---|---|
| Default | 24px | 18px | 22px | 2px | ❌ 1px (off by 1) |
| Small | 18px | 14px | 16px | 1px | ✅ correct |
| Large | 32px | 24px | 30px | 3px | ❌ 1px (off by 2) |
| Longswipe+small | 18px | 14px | 16px | 1px | ✅ correct |
| Longswipe+large | 32px | 24px | 30px | 3px | ❌ 1px (off by 2) |
So after the revert: default and large are misaligned; small is actually fine. Let me update the issue to reflect this accurately.
🏁 Script executed:
#!/bin/bash
gh issue edit 5 --repo FlowingCode/ToggleButton \
--title "fix: readonly slider vertical alignment broken for default and large size variants" \
--body "## Summary
In readonly state, \`.switch\` gains a \`1px dashed\` border with \`box-sizing: border-box\`, which reduces the inner track height by 2px (1px top + 1px bottom). The \`:host([readonly]) .slider\` rule keeps \`top: 1px\` (reverted from the attempted \`top: 2px\` fix in PR `#4`), meaning the slider knob is not vertically centred for the default and large sizes.
## Root Cause
The correct \`top\` offset in readonly state must account for the 1px border:
\`\`\`
correct_top = (switch_height - 2px_border - slider_height) / 2
\`\`\`
## Expected vs Current Behaviour
| Size | Switch H | Slider H | Correct readonly \`top\` | Current \`top: 1px\` |
|---|---|---|---|---|
| Default / medium | 24px | 18px | **2px** | ❌ 1px (1px too high) |
| Small | 18px | 14px | **1px** | ✅ correct |
| Large | 32px | 24px | **3px** | ❌ 1px (2px too high) |
| Longswipe + small | 18px | 14px | **1px** | ✅ correct |
| Longswipe + large | 32px | 24px | **3px** | ❌ 1px (2px too high) |
## Proposed Fix
Set the base readonly rule to the correct default value and add per-size overrides for large:
\`\`\`css
:host([readonly]) .slider {
top: 2px; /* default & medium: (22 - 18) / 2 = 2px */
left: 1px;
box-shadow: none;
background-color: var(--lumo-contrast-40pct, var(--vaadin-background-container-strong));
}
/* small inner height = 16px → (16 - 14) / 2 = 1px */
:host([readonly][theme~=\"small\"]) .slider,
:host([readonly][theme~=\"longswipe\"][theme~=\"small\"]) .slider {
top: 1px;
}
/* large inner height = 30px → (30 - 24) / 2 = 3px */
:host([readonly][theme~=\"large\"]) .slider,
:host([readonly][theme~=\"longswipe\"][theme~=\"large\"]) .slider {
top: 3px;
}
\`\`\`
## Context
Identified during review of PR `#4` (https://github.com/FlowingCode/ToggleButton/pull/4) — specifically the discussion at https://github.com/FlowingCode/ToggleButton/pull/4#discussion_r3229628971.
The \`top: 2px\` fix attempted in PR `#4` was reverted because it exposed the missing per-size overrides for \`large\`. This issue tracks the complete fix covering all size variants.
Reported by `@paodb`."
Length of output: 53
@paodb, you're correct — the original issue framing was inaccurate because it was written assuming top: 2px was in place. With the revert back to top: 1px, the situation is actually different:
- Small (and
longswipe + small):top: 1pxis already correct for readonly ✅ - Default / medium:
top: 1pxis 1px too high — needstop: 2px❌ - Large (and
longswipe + large):top: 1pxis 2px too high — needstop: 3px❌
The border arithmetic is: correct_top = (switch_height − 2 × border − slider_height) / 2. The 1px dashed border eats 2px of inner space, so every size needs its top bumped by 1px relative to the non-readonly value.
I've updated issue #5 with the corrected analysis table and the revised proposed fix. The complete fix will be: base rule top: 2px, a top: 1px override for small, and a top: 3px override for large.
There was a problem hiding this comment.
Now it looks good, thanks!
paodb
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
github-project-automation
Bot
moved this from To Do
to Pending release
in Flowing Code Addons
3 participants
This PR fixes visual issues in
ToggleButton's read-only state:Color variant overrides readonly styling: when a color variant (
PRIMARY,SUCCESS,WARNING,ERROR,CONTRAST) was set on a read-only checked toggle, the variant's solid background overrode the dashed-border read-only look. New per-variant rules at higher CSS specificity now force the transparent background and tint the dashed border with a 40%-desaturatedcolor-mix()of the variant color. The rule only triggers when both[checked]and[readonly]are set. Toggles without a color variant keep the neutral read-only border. Lumo and Aura compatibility preserved via the existingvar(--lumo-*, var(--aura-*))fallback pattern.Close #2
Summary by CodeRabbit