chore: improve toggle transition on touch devices#2742
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughToggle input classes are changed to apply hover and checked-hover Tailwind variants only under ChangesToggle Accessibility & Layout
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
app/components/Settings/Toggle.client.vue (1)
44-44:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winApply the same hover media query fix to this input.
This toggle input in the
reverseOrdertemplate branch still uses unconditionalhover:andchecked:hover:utilities, which will cause the same sticky hover issue on touch devices that the PR aims to fix. Only the input at line 90 has been updated with the[@media(hover:hover)]media query wrapper.To fix the issue consistently across both rendering paths, apply the same media query variants here.
🔧 Proposed fix to match line 90
- class="toggle appearance-none h-6 w-11 rounded-full border border-fg relative shrink-0 bg-fg/50 transition-colors duration-200 ease-in-out checked:bg-fg hover:bg-fg/60 checked:hover:(bg-fg/80 after:opacity-50) focus-visible:(outline-2 outline-accent outline-offset-2) before:(content-[''] absolute h-5 w-5 top-px start-px rounded-full bg-bg transition-transform duration-200 ease-in-out) checked:before:translate-x-[20px] rtl:checked:before:-translate-x-[20px] after:(content-[''] absolute h-3.5 w-3.5 top-[4px] start-[4px] i-lucide:check bg-fg opacity-0 transition-all duration-200 ease-in-out) checked:after:opacity-100 checked:after:translate-x-[20px] rtl:checked:after:-translate-x-[20px]" + class="toggle appearance-none h-6 w-11 rounded-full border border-fg relative shrink-0 bg-fg/50 transition-colors duration-200 ease-in-out checked:bg-fg [`@media`(hover:hover)]:hover:bg-fg/60 [`@media`(hover:hover)]:checked:hover:(bg-fg/80 after:opacity-50) focus-visible:(outline-2 outline-accent outline-offset-2) before:(content-[''] absolute h-5 w-5 top-px start-px rounded-full bg-bg transition-transform duration-200 ease-in-out) checked:before:translate-x-[20px] rtl:checked:before:-translate-x-[20px] after:(content-[''] absolute h-3.5 w-3.5 top-[4px] start-[4px] i-lucide:check bg-fg opacity-0 transition-all duration-200 ease-in-out) checked:after:opacity-100 checked:after:translate-x-[20px] rtl:checked:after:-translate-x-[20px]"🤖 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 `@app/components/Settings/Toggle.client.vue` at line 44, In Settings/Toggle.client.vue the toggle input in the reverseOrder template still uses unconditional hover: and checked:hover: utilities causing sticky hover on touch devices; update the same input (the element with class starting "toggle appearance-none h-6 w-11...") to wrap hover and checked:hover variants with the [`@media`(hover:hover)] media query just like the other input at line 90 so hover: and checked:hover: only apply on devices that support hover.
🧹 Nitpick comments (1)
app/components/Settings/Toggle.client.vue (1)
38-93: ⚖️ Poor tradeoffConsider refactoring to reduce template duplication.
The component duplicates the toggle input and label markup across both
reverseOrderbranches (lines 38-64 vs. 66-92). This duplication increases maintenance burden and can lead to inconsistencies, as evidenced by the hover fix needing to be applied in two places.Consider refactoring to use computed properties for grid positioning and conditional ordering whilst keeping the markup DRY.
🤖 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 `@app/components/Settings/Toggle.client.vue` around lines 38 - 93, The template duplicates the input and label markup for the two branches controlled by reverseOrder; refactor by rendering the input (id, v-model="checked", role="switch") and the label/TooltipApp (uses label, tooltip, tooltipPosition, tooltipTo, tooltipOffset) only once and compute their order/position via computed properties or conditional bindings (e.g., computed gridArea/justifySelf and an inputClass string that switches the hover media selector). Update the component to expose a computed like inputStyle/inputClass and labelStyle/labelWrapper that choose grid-area and justify-self based on reverseOrder so the same markup is reused and TooltipApp logic remains unchanged.
🤖 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.
Outside diff comments:
In `@app/components/Settings/Toggle.client.vue`:
- Line 44: In Settings/Toggle.client.vue the toggle input in the reverseOrder
template still uses unconditional hover: and checked:hover: utilities causing
sticky hover on touch devices; update the same input (the element with class
starting "toggle appearance-none h-6 w-11...") to wrap hover and checked:hover
variants with the [`@media`(hover:hover)] media query just like the other input at
line 90 so hover: and checked:hover: only apply on devices that support hover.
---
Nitpick comments:
In `@app/components/Settings/Toggle.client.vue`:
- Around line 38-93: The template duplicates the input and label markup for the
two branches controlled by reverseOrder; refactor by rendering the input (id,
v-model="checked", role="switch") and the label/TooltipApp (uses label, tooltip,
tooltipPosition, tooltipTo, tooltipOffset) only once and compute their
order/position via computed properties or conditional bindings (e.g., computed
gridArea/justifySelf and an inputClass string that switches the hover media
selector). Update the component to expose a computed like inputStyle/inputClass
and labelStyle/labelWrapper that choose grid-area and justify-self based on
reverseOrder so the same markup is reused and TooltipApp logic remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7cff39e0-410a-4dd8-bc2f-9295ee126176
📒 Files selected for processing (1)
app/components/Settings/Toggle.client.vue
There was a problem hiding this comment.
Oops, good catch, thanks, done
There was a problem hiding this comment.
@alexdln It looks like both classes are identical. Could it be simplified/deduplicated somehow to not have to update both at the same time and not forget to?
There was a problem hiding this comment.
I'm honestly not even sure why there are two variations here... It seems like it could be solved with classes. I'll check it out tomorrow 👀
There was a problem hiding this comment.
Yes, this was already done via styles before, but it still changed the semantic order. Essentially, it was only for crawlers/readers. And I think it's unnecessary, since even for a11y, it's generally more useful to leave the text at the beginning. Removed condition
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 `@app/components/Settings/Toggle.client.vue`:
- Line 61: The class string in Toggle.client.vue uses invalid UnoCSS arbitrary
variant syntax like [`@media`(hover:hover)]:hover:bg-fg/60 and
[`@media`(hover:hover)]:checked:hover:..., which UnoCSS doesn't support; replace
these with a proper custom variant or rules in your UnoCSS config and then use
that variant name in the class list (e.g., define a variant such as
"hover-capable" in config.variants that emits `@media` (hover: hover) { & :hover {
... } } and update the class tokens (the long class attribute on the toggle
input) to use that variant instead of [`@media`(...)] so hover styles are
correctly gated by the media query.
🪄 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: CHILL
Plan: Pro
Run ID: 418a13cf-d65a-4a59-a46e-e602e24c0b09
📒 Files selected for processing (2)
app/components/Settings/Toggle.client.vueapp/components/Settings/Toggle.server.vue
3 participants
🔗 Linked issue
Resolves #2540
🧭 Context
A micro-change to improve hover effects. I simply disabled this effect on touch devices (for checked checkbox on hover). Tailwind seems to be able to do this automatically, but it doesn't work for us. Current change should be more than enough for now