-
Notifications
You must be signed in to change notification settings - Fork 702
feat(CheckboxGroup): new component #3862
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
base: v3
Are you sure you want to change the base?
Conversation
@benjamincanac you know I'm a super fan of this π¬ (#895) EDIT: oh man, fall 2023, time flies... |
commit: |
{{ legend }} | ||
</slot> | ||
</legend> | ||
<component :is="variant === 'list' ? 'div' : Label" v-for="item in normalizedItems" :key="item.value" :class="ui.item({ class: props.ui?.item })"> |
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.
Is there a reason to not use the UCheckbox
component here instead of re-implementing everything? π€
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.
Good question! I started off with the intention of using the UCheckbox component. I felt that making the component as close to URadioGroup in structure was important to ensure consistency in behaviour in the new variants and how they applied to each component. As I proceeded, I became more confident I could achieve that aim by re-implementing instead. I'm willing to defer to your experience and refactor as suggested - this is my first pull request in any codebase so I'm more than happy to be guided.
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.
We can left it like that I guess but I would share the theme between checkbox
and checkbox-group
though. The new variants should also be available on Checkbox.
You can check how textarea
imports input
theme for example.
Co-authored-by: Benjamin Canac <[email protected]>
|
||
Use the `items` prop as an array of strings or numbers: | ||
|
||
::component-code |
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.
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.
Hi @romhml. I have not been able to get pnpm run docs
to work at any stage of working on this (JS heap out of memory). Is there another way I can review the page so I can see the behaviour you've seen?
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.
This was caused by an invalid modelValue
in the docs, I fixed it here: 7a7180c
Co-authored-by: Romain Hamel <[email protected]>
Co-authored-by: Romain Hamel <[email protected]>
π Linked issue
Resolves #3050
β Type of change
π Description
The UCheckbox component currently handles single instances of a checkbox only, returning a string. Being able to put several checkboxes together and returning an array is a common user requirement that has already been implemented in reka-ui but not yet in Nuxt UI. The new component builds on the reka example and adheres to the same patterns found in the existing URadioGroup component.
#3178 has added some beautiful and very useful styling for the URadioGroup component via the new
table
&card
variants and so these same variants have been implemented in the new UCheckboxGroup component too.π Checklist