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! |
📝 WalkthroughWalkthroughThe toggle component’s client markup was refactored from an inline input to a composed control: the label now contains a hidden checkbox input, a Track span and a Thumb/Check icon with grid-area placement and RTL-aware classes; related scoped styles were removed and CSS selectors were updated (including forced-colours/forced-colors handling) to target the new Track/Thumb structure. The server component’s props were extended with Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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 */
}2 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):