-
-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Checkbox group component #502
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for kobalte ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking time to submit this PR!
However a few changes are needed; most of these components are the same as Checkbox
but duplicated and renamed.
These can be re-exported and renamed instead of having a duplicate of each, an example of this can be seen in DropdownMenu
which re-exports Menu
(example).
This most likely applies to -item-control
, -item-description
, -item-indicator
, -item
and -item-label
.
For the behavior of the CheckboxGroup.Item
it would be better to return a wrapped Checkbox.Root
rather than a copy with modified behavior.
I'd suggest inject the group status inside the normal checkbox through checked={checkboxGroupContext.value().includes(<self value>)}
and onChange={(checked) => checkboxGroupContext.setSelected(<self value>, checked)}
(pseudo code, replace with actual implementation).
Example here.
Also it seems like the HTML forms example is broken.
Let me know if anything needs to be clarified.
Thanks again for the PR.
Thanks for the information and review @jer3m01 |
Hi @jer3m01, the component is almost complete. However, I’m stuck on some issues:
I’d appreciate it if you could take a look and let me know if I missed anything. |
closes #310