Skip to content

Conversation

@abbeyperini
Copy link

@abbeyperini abbeyperini commented Feb 5, 2026

Update settings page toggle to use an <input type="checkbox" /> instead of a button. Wrap the whole thing in a label and hide the checkbox behind styled toggle, so the click/touch target is pretty huge. Use translate animation and style it to look about the same as before. Use theme color so all color modes are supported. Use locale dir to support right to left languages.

Closes: #1028

@vercel
Copy link

vercel bot commented Feb 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 11, 2026 5:51pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 11, 2026 5:51pm
npmx-lunaria Ignored Ignored Feb 11, 2026 5:51pm

Request Review

@codecov
Copy link

codecov bot commented Feb 5, 2026

Codecov Report

❌ Patch coverage is 58.82353% with 7 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
app/components/Settings/Toggle.server.vue 55.55% 4 Missing ⚠️
app/components/Settings/Toggle.client.vue 62.50% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Contributor

@knowler knowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change. I noticed these had some affordances I don’t typically associate with switches (i.e. activation on Enter).

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 10, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Replaces the button-based Settings Toggle with a label+native input type="checkbox" implementation using v-model. Internal model declaration now uses definedModel<boolean> with required: true and an id is generated for label association. Templates and styles were rewritten to a CSS-grid layout with toggle-default / toggle-reverse classes, a toggle-background element and a moving thumb implemented with pseudo-elements; inputs are visually hidden but retain role="switch" and ARIA sync. Server-side skeleton template was adjusted; tests now mount with an explicit modelValue: false where applicable.

Possibly related PRs

Suggested reviewers

  • danielroe
  • alexdln
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description directly aligns with the changeset, detailing the replacement of a button-based toggle with a checkbox-based implementation wrapped in a label, animation styling, theme colour support, and locale direction handling.
Linked Issues check ✅ Passed The pull request successfully addresses all coding requirements from issue #1028: checkbox input replaces button, label wrapper is implemented, focus behaviour is preserved, a11y tests are updated, and CSS selectors enable richer styling options.
Out of Scope Changes check ✅ Passed All modifications remain focused on the toggle component refactoring and related test updates specified in issue #1028; no extraneous changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

@danielroe
Copy link
Member

this is a great change!

just one comment: the radii/position of the dot isn't quite right. here's a comparison

before SCR-20260211-baky
after SCR-20260211-baty

Copy link
Contributor

@knowler knowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good! Works as expected. I’ve got a couple of requests (incorporating the forced color mode styles and using :dir() instead of a class) and a few nits.

I didn’t leave it as a comment, but I also wonder if we should use logical properties for the dimensions (i.e. height, width as block-size, inline-size respectively). Reason being is that grid/flex layout are writing-mode/directionality aware.

Comment on lines 55 to 151
@media (forced-colors: active) {
/* make toggle tracks and thumb visible in forced colors. */
button[role='switch'] {
& > span:last-of-type {
forced-color-adjust: none;
}
&[aria-checked='false'] > span:last-of-type {
background: Canvas;
border-color: CanvasText;
& > span {
background: CanvasText;
}
}
&[aria-checked='true'] > span:last-of-type {
background: Highlight;
border-color: Highlight;
& > span {
background: HighlightText;
}
}
}
.toggle--checkbox:checked.rtl + .toggle--background:before {
transform: translate(-17px);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine this might have gotten dropped when merging as I believe it was added after you started your PR. Can we incorporate it somehow? If you don’t have Windows, you can emulate forced colours in Chrome devtools.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has it not been incorporated into the theme in main.css? We should also add it to the tests since we're already doing dark and light mode

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has it not been incorporated into the theme in main.css?

It probably should be, but it isn’t right now (there are some forced colours styles there, but they don’t apply to this).

Copy link
Contributor

@knowler knowler Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see @media (prefers-contrast: more) for some styles, but that doesn’t apply for forced colours unfortunately. Why don’t we create a separate PR for that and we can ignore here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I was suggesting issues, not volunteering lol

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a bug - in high contrast mode, the label isn't showing 1/2 the time. I'll fix that when I finish these styles in the morning.

Screen.Recording.2026-02-10.at.8.10.55.PM.mov

Copy link
Author

@abbeyperini abbeyperini Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok the latest commit should fix forced contrast styles. I will accept style tag to inline utility class translations if you've got them.

Screen.Recording.2026-02-11.at.12.49.29.PM.mov

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, my friend made this excellent Color Blind & Low Vision Simulator Chrome Extension

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick test with Windows High Contrast Mode (Night sky theme) and Edge on Windows 11 and this is what I found:

  1. Looks like the label disappears when its enabled.
  2. There’s also a slight flicker for the “thumb” when it changes colour.
  3. When enabled the background and text colour is probably failing the minimum contrast ratio.
npmx-switch-forced-colors-win11-edge.mp4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that when using a light contrast theme (e.g. “Desert”) these issues aren’t present, but I might still avoid the label colour inversion:

The “Display” settings for NPMX shown using the Desert theme for Windows High Contrast Mode. The labels are inverted when the control is enabled, but the contrast of everything is fine.

abbeyperini and others added 5 commits February 10, 2026 19:15
Co-authored-by: Nathan Knowler <nathan@knowler.dev>
Co-authored-by: Nathan Knowler <nathan@knowler.dev>
Co-authored-by: Nathan Knowler <nathan@knowler.dev>
Co-authored-by: Nathan Knowler <nathan@knowler.dev>
Co-authored-by: Nathan Knowler <nathan@knowler.dev>
Co-authored-by: Nathan Knowler <nathan@knowler.dev>
Copy link
Contributor

@knowler knowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one last set of changes all related to the transitions.

abbeyperini and others added 2 commits February 10, 2026 20:08
Co-authored-by: Nathan Knowler <nathan@knowler.dev>
Co-authored-by: Nathan Knowler <nathan@knowler.dev>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
app/components/Settings/Toggle.client.vue (2)

7-7: Redundant defineEmits when using defineModel.

defineModel internally handles the update:modelValue emit, so the explicit defineEmits declaration is unnecessary and can be removed to reduce noise.

♻️ Suggested change
-defineEmits(['update:modelValue'])

2-6: Unused class prop.

The class prop is defined but never applied in the template. When explicitly declared as a prop, Vue disables automatic class fallthrough, so any class passed by consumers will have no effect. Either remove the prop or apply it to the appropriate element (likely the outer <label>).

♻️ Option A: Apply the class prop
 <template>
-  <label :for="id">
+  <label :for="id" :class="props.class">
♻️ Option B: Remove unused prop
 const props = defineProps<{
   label: string
   description?: string
-  class?: string
 }>()

Copy link
Contributor

@knowler knowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! As usual, I’ll defer to others with regard to Vue related stuff.

I will follow up with the forced colors mode stuff at the theme-level.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: use input-checkbox for toggle instead of button

3 participants