Skip to content

Conversation

@ttaylor-stack
Copy link
Collaborator

@ttaylor-stack ttaylor-stack commented Nov 21, 2025

Fri Dec 12, 2025 updates from @dancormier

I made some significant changes to this PR and the overall structure of checkbox/radio and related elements/components. Here are the notable changes in this PR:

  • Replaced .s-check-group with .s-check, which now contains all styling applied to child checkbox and radio elements
  • Removed .s-checkbox and .s-radio classes. The correct styling will apply as long as the parent includes .s-check
  • Added Check and CheckGroup Svelte components
  • Renames "Checkbox & Radio" docs page to "Check"

Links

Stacks Classic

Stacks Svelte

Screenshots

Checkbox

image

Radio

image

Checkmark

image

Original PR description

PR to update radio and checkbox components to SHINE designs. Added Checkmark component too.

Stacks Svelte components created for all of the above

Menu component updated to use checkmark styling instead of custom styling

Story: https://stackoverflow.atlassian.net/browse/SPARK-77
Figma: https://www.figma.com/design/do4Ug0Yws8xCfRjHe9cJfZ/Project-SHINE---Product-UI?node-id=2344-22923&m=dev

NOTE: I haven't updated the migration guide because there are no breaking changes and I don't think its needed for new components. Let me know if this is correct

@CGuindon , I spoke to Dan and decided to remove the Focus column in the docs, with the way the browser works there's no easy way to get several elements to focus at once

@changeset-bot
Copy link

changeset-bot bot commented Nov 21, 2025

🦋 Changeset detected

Latest commit: 9113a05

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Nov 21, 2025

Deploy Preview for stacks-svelte ready!

Name Link
🔨 Latest commit 9113a05
🔍 Latest deploy log https://app.netlify.com/projects/stacks-svelte/deploys/6949950eb70972000857ea2a
😎 Deploy Preview https://deploy-preview-2060--stacks-svelte.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link

netlify bot commented Nov 21, 2025

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 9113a05
🔍 Latest deploy log https://app.netlify.com/projects/stacks/deploys/6949950edda00f00081e8ac7
😎 Deploy Preview https://deploy-preview-2060--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@ttaylor-stack ttaylor-stack changed the base branch from develop to beta November 21, 2025 16:17
Copy link
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

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

Comment hidden by @dancormier to save me from having to scroll so much 😂
  1. Screenshot 2025-11-21 at 10 06 14 AM

I'm tempted to just call this "Checkbox & Radio" — that's more common to the component function itself that would be more familiar. @dancormier @giamir Thoughts and preferences?


  1. Screenshot 2025-11-21 at 10 11 07 AM

For the checkmark example, I can select the unselected ones but then I can deselect/uncheck any. I think we should make this available to test out (I can check and uncheck them examples) similar to Radio and Checkmark.


  1. Screenshot 2025-11-21 at 10 15 26 AM

For the Vertical group example, let's add the example I have in Figma with checkmarks as well below the others

(not needed for horiztonal)

Same for With Description Copy example — include checkmarks version.
Screenshot 2025-11-21 at 10 17 35 AM

@CGuindon
Copy link
Collaborator

CGuindon commented Nov 21, 2025

Deploy for Svelte preview failed....?

Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Love this second iteration @dancormier. I really like to see us moving closer to what other systems are doing with a clear distinction between radio and checkbox at the API level for consumers. I think this is a huge devx improvement! ❤️

I think the PR is almost ready to be merged.
I have 2 major suggestions:

  • Remove the CheckboxGroup and RadioGroup separate stories files since they don't make sense in isolation (instead just add one or 2 extra stories to the Checkbox and Radio stories the Group component could be added as a subcomponent in the story file) . In fact what about keeping things in the same folder as well (Radio contains Radio and RadioGroup - Checkbox contains Checkbox and CheckboxGroup - [Internal]Check contains Check and CheckGroup). I think this will simplify our structure a bit and the amount of code we add to the repo.

  • Currently the Radio and Checkbox components don't have a way for consumers to listen to user changes. We need to be probably do a couple of things:

    • Make the checked prop $bindable to the underling input element so that consumers can listen to user changing it and react accordingly in their app <Checkbox bind:checked /> (you can check what they are doing in this system as a reference)
    • We should probably also add an onchange prop as an alternative way for consumer to react to the changes the user make (this would be the one way binding popularized by React) <Checkbox onchange={() => 'update state'} />
    • Add one or two stories (and one or two tests) showcasing how a consumer will listen to the user changes (either with two way binding $bindable or one way binding onchange)
  • Nice to have (for a follow up ticket/PR): RadioGroup and CheckboxGroup could be smarter for our consumers and do quite a bit of the heavy lifting for them. We could add to them a value and onValueChange prop which will make life for consumers so much easier. As I said though this is something we can iterate on after. Maybe we can create a ticket though for it.

<RadioGroup value={state} onValueChange={(newState) => state = newState}>
   <Radio name="group" id="banana" label="Banana" />
   <Radio name="group" id="apple" label="Apple" />
   <Radio name="group" id="pear" label="Pear" />
</RadioGroup>

instead of

<RadioGroup>
  <Radio name="group" id="banana" label="Banana" checked={state === "banana"} onchange={() => state = "banana"} />
  <Radio name="group" id="apple" label="Apple" checked={state === "apple"} onchange={() => state = "apple"} />
  <Radio name="group" id="pear" label="Pear" checked={state === "pear"} onchange={() => state = "pear"} />
</RadioGroup>

The former version is much nicer for consumers (but they can still get the job done with latter one for now).

Happy to chat sync about my suggestions if something is unclear. Thanks again for getting us closer to industry standards with this PR. ❤️

@dancormier
Copy link
Contributor

Love this second iteration @dancormier. I really like to see us moving closer to what other systems are doing with a clear distinction between radio and checkbox at the API level for consumers. I think this is a huge devx improvement! ❤️

I think the PR is almost ready to be merged. I have 2 major suggestions:

* **Remove the CheckboxGroup and RadioGroup separate stories files** since they don't make sense in isolation (instead just add one or 2 extra stories to the Checkbox and Radio stories the Group component could be added as a subcomponent in the story file) . In fact what about keeping things in the same folder as well (Radio contains Radio and RadioGroup - Checkbox contains Checkbox and CheckboxGroup - [Internal]Check contains Check and CheckGroup). I think this will simplify our structure a bit and the amount of code we add to the repo.

✅ I took inspiration from a few other component libraries (bits-ui, MUI, shadcn/ui) and included story files for Checkbox and RadioGroup. Checkbox includes a story demonstrating CheckboxGroup and RadioGroup includes a story for Radio. I also consolidated files into directories /Checkbox and /RadioGroup and updated tests files to include related components.


* **Currently the Radio and Checkbox components don't have a way for consumers to listen to user changes**. We need to be probably do a couple of things:
  
  * Make the `checked` prop `$bindable` to the underling input element so that consumers can listen to user changing it and react accordingly in their app `<Checkbox bind:checked />` (you can check what they are doing in [this system](https://bits-ui.com/docs/components/checkbox#api-reference) as a reference)
  * We should probably also add an `onchange` prop as an alternative way for consumer to react to the changes the user make (this would be the one way binding popularized by React)  `<Checkbox onchange={() => 'update state'} />`
  * Add one or two stories (and one or two tests) showcasing how a consumer will listen to the user changes (either with two way binding `$bindable` or one way binding `onchange`)

✅ I've added $bindable and an onchange prop, as well as stories to demonstrate.

* Nice to have (for a follow up ticket/PR): RadioGroup and CheckboxGroup could be smarter for our consumers and do quite a bit of the heavy lifting for them. We could add to them a `value` and `onValueChange` prop which will make life for consumers so much easier. As I said though this is something we can iterate on after. Maybe we can create a ticket though for it.
<RadioGroup value={state} onValueChange={(newState) => state = newState}>
   <Radio name="group" id="banana" label="Banana" />
   <Radio name="group" id="apple" label="Apple" />
   <Radio name="group" id="pear" label="Pear" />
</RadioGroup>

instead of

<RadioGroup>
  <Radio name="group" id="banana" label="Banana" checked={state === "banana"} onchange={() => state = "banana"} />
  <Radio name="group" id="apple" label="Apple" checked={state === "apple"} onchange={() => state = "apple"} />
  <Radio name="group" id="pear" label="Pear" checked={state === "pear"} onchange={() => state = "pear"} />
</RadioGroup>

The former version is much nicer for consumers (but they can still get the job done with latter one for now).

✅ I took this just a little further by adding an options prop to RadioGroup and CheckboxGroup so consumers can just pass an array of options. I've also added onValueChange to the underlying FormGroup so that consumers can execute functions when the value changes. If this isn't solid, let me know and I'll remove it for now and leave it for a round 2.

Happy to chat sync about my suggestions if something is unclear. Thanks again for getting us closer to industry standards with this PR. ❤️

@giamir Let me know how this PR looks after this last round of changes (it can wait until the new year… please don't rush to review!). It's a pretty beefy PR, so let me know if you think it needs to be split up at all and I'm happy to do so.

@dancormier dancormier requested review from giamir and mukunku December 19, 2025 00:18
Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Thanks Dan for addressing my comments so quickly. I have taken a quick second look and I would be happy to see this merged. The only NIT I have is to add CheckboxGroup and Radio as subcomponents in the Checkbox and RadioGroup stories respectively. This will allow consumers to consult the API of those components too if they need to.

Thanks for the great work ❤️

</Popover>
</Story>

<!-- TODO SHINE Update example -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO still relevant?

@ttaylor-stack ttaylor-stack enabled auto-merge (squash) December 22, 2025 19:00
@ttaylor-stack ttaylor-stack merged commit d53cdd1 into beta Dec 22, 2025
18 checks passed
@ttaylor-stack ttaylor-stack deleted the spark-77/update-radio-and-checkbox-components-for-shine branch December 22, 2025 19:06
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.

6 participants