Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe client Toggle component markup was refactored: the label now wraps the checkbox input, Track span and Thumb/Check visuals with updated grid placement, RTL-aware classes and transition-friendly utility classes; the previous scoped toggle styles were removed and replaced by a new style block targeting the Track/Thumb structure and forced‑colour/reduced‑motion selectors. The server-side Toggle component’s props were extended to include Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (1)
app/components/Settings/Toggle.client.vue (1)
128-134: Scope reduced-motion rules to toggle elements only.Using
span/span::afterhere is broader than necessary and may suppress transitions on unrelated spans added later in this component.Refactor suggestion
`@media` (prefers-reduced-motion: reduce) { - span, - span::after, + input + span, + input + span::after, + input + span + span, .i-lucide\:check { transition: none !important; animation: none !important; } }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Settings/Toggle.client.vue (1)
150-156: Scope reduced-motion rules to toggle parts onlyLine [151] currently targets every
spanin this component scope, which is broader than needed. Narrowing it to the switch elements avoids accidental side effects on non-toggle spans.Suggested refactor
-@media (prefers-reduced-motion: reduce) { - span, - span::after, - .i-lucide\:check { +@media (prefers-reduced-motion: reduce) { + input + span, + input + span::after, + .toggle-thumb-icon, + .i-lucide\:check { transition: none !important; animation: none !important; } }- <span - class="absolute top-[2px] start-[2px] h-5 w-5 rounded-full flex items-center justify-center transition-transform duration-200 ease-in-out pointer-events-none" + <span + class="toggle-thumb-icon absolute top-[2px] start-[2px] h-5 w-5 rounded-full flex items-center justify-center transition-transform duration-200 ease-in-out pointer-events-none" :class="checked ? 'translate-x-5 rtl:-translate-x-5' : 'translate-x-0'" >
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/components/Settings/Toggle.client.vue (2)
62-107: Remove duplicated reverse-order label blockBoth
props.reverseOrderandv-elsebranches render the same tooltip/label markup. Keeping a single block will reduce template complexity and future drift risk.♻️ Proposed simplification
- <template v-if="props.reverseOrder"> - <TooltipApp - v-if="tooltip && label" - :text="tooltip" - :position="tooltipPosition ?? 'top'" - :to="tooltipTo" - :offset="tooltipOffset" - > - <span - class="toggle-label text-sm text-fg font-medium text-start" - style="grid-area: label-text" - > - {{ label }} - </span> - </TooltipApp> - <span - v-else-if="label" - class="toggle-label text-sm text-fg font-medium text-start" - style="grid-area: label-text" - > - {{ label }} - </span> - </template> - <template v-else> - <TooltipApp - v-if="tooltip && label" - :text="tooltip" - :position="tooltipPosition ?? 'top'" - :to="tooltipTo" - :offset="tooltipOffset" - > - <span - class="toggle-label text-sm text-fg font-medium text-start" - style="grid-area: label-text" - > - {{ label }} - </span> - </TooltipApp> - <span - v-else-if="label" - class="toggle-label text-sm text-fg font-medium text-start" - style="grid-area: label-text" - > - {{ label }} - </span> - </template> + <TooltipApp + v-if="tooltip && label" + :text="tooltip" + :position="tooltipPosition ?? 'top'" + :to="tooltipTo" + :offset="tooltipOffset" + > + <span + class="toggle-label text-sm text-fg font-medium text-start" + style="grid-area: label-text" + > + {{ label }} + </span> + </TooltipApp> + <span + v-else-if="label" + class="toggle-label text-sm text-fg font-medium text-start" + style="grid-area: label-text" + > + {{ label }} + </span>
46-52: Scope reduced-motion selectors to toggle elements onlyThe reduced-motion rule currently targets every
spanin the component. Narrowing this to toggle-specific elements will prevent accidental side effects as the template evolves.🎯 Proposed selector scoping
- <span - class="w-11 h-6 bg-fg-subtle peer-focus:outline-none peer-focus-visible:ring-2 peer-focus-visible:ring-fg peer-focus-visible:ring-offset-2 rounded-full peer peer-checked:after:translate-x-full rtl:peer-checked:after:-translate-x-full peer-checked:after:border-white after:content-[''] after:absolute after:top-[2px] after:start-[2px] after:bg-bg after:border-gray-300 after:border after:rounded-full after:h-5 after:w-5 after:transition-all peer-checked:bg-fg transition-colors duration-200 ease-in-out" - ></span> + <span + class="toggle-track w-11 h-6 bg-fg-subtle peer-focus:outline-none peer-focus-visible:ring-2 peer-focus-visible:ring-fg peer-focus-visible:ring-offset-2 rounded-full peer peer-checked:after:translate-x-full rtl:peer-checked:after:-translate-x-full peer-checked:after:border-white after:content-[''] after:absolute after:top-[2px] after:start-[2px] after:bg-bg after:border-gray-300 after:border after:rounded-full after:h-5 after:w-5 after:transition-all peer-checked:bg-fg transition-colors duration-200 ease-in-out" + ></span> @@ - <span - class="absolute top-[2px] start-[2px] h-5 w-5 rounded-full flex items-center justify-center transition-transform duration-200 ease-in-out pointer-events-none" + <span + class="toggle-thumb absolute top-[2px] start-[2px] h-5 w-5 rounded-full flex items-center justify-center transition-transform duration-200 ease-in-out pointer-events-none" :class="checked ? 'translate-x-5 rtl:-translate-x-5' : 'translate-x-0'" > @@ `@media` (prefers-reduced-motion: reduce) { - span, - span::after, + .toggle-track, + .toggle-track::after, + .toggle-thumb, .i-lucide\:check { transition: none !important; animation: none !important; } }Also applies to: 156-162
knowler
left a comment
knowler
left a comment
There was a problem hiding this comment.
I will do a fuller review tomorrow, but I did notice the focus outlines are missing.
It would be very nice if we could avoid using the extra <span> elements. I imagine you went that route so that you could use icons, but I’m curious if there’s a way to use the icon information without the classes as it complicates the whole focus/activation situation. If we could use it in the content of one of the pseudo-elements of the <input> that would be ideal.
/* track */
input {}
/* thumb */
input::after {
content: url(); /* checkmark data URI */
}
|
I am not sure the icon is a right change... It's a toggle, not a checkbox. And imo, the icon confuses the experience a bit more than it helps. Regarding the implementation, if knowler will find it as important change - please add hovers (for the off and on states) Some another option: P.S. To implement the message above, you can use something like: |
|
|
In #1782 (comment), @alexdln said:
Hover states are a good idea, too. With regard to using an icons… switches are a difficult control to get right. Technically, they do pass something like WCAG’s “Use of color” success criterion, since the position of the thumb is enough to differentiate between the two states, but what’s still not immediately clear without colour is which position is on or off. That’s where icons come in. It seems like usually, the icon isn’t placed on the thumb, but within the track (see “Toggle” on Components Gallery or Open UI’s research for “Switch” or try the “On/Off Labels” setting in the “Display & Text Size” accessibility settings on iOS if you have such a device). Even then, I find icons don’t fully clear up the confusion (e.g. does the icon mean that’s the state it will be in if I toggle it?). So, I don’t have any strong opinions about the necessity of including them. One of those things that could go either way. Some resources as I find them: |
|
We can use "segmented buttons" (from md3 expressive) to solve the issue with the meaning (with enabled/disabled texts): https://m3.material.io/components/segmented-buttons/overview |
3 participants
🔗 Linked issue
resolves #1758
🧭 Context
This PR change the component layout to add the checked icon using position absolute.
📚 Description
This PR includes:
With forced colors active and RTL locale (same for non RTL):