radix-vue / shadcn-vue

Vue port of shadcn-ui
https://www.shadcn-vue.com/
MIT License
3.51k stars 203 forks source link

[Bug]: v-model doesn't work on Checkbox #622

Open sugoidesune opened 1 week ago

sugoidesune commented 1 week ago

Reproduction

https://stackblitz.com/edit/nce3t7?file=src%2FApp.vue

Describe the bug

<Checkbox v-model="toggle" />
{{ toggle }}

Doesn't sync state with the toggle variable.

System Info

shadcn-vue 10.05

Contributes

rodrigo-fantuci commented 1 week ago

See 121

sugoidesune commented 1 week ago

My solution now was the following: This keeps the original and adds support for v-model

I think it's more than warranted to bring back expected vue functionality and dev experience even if shadcn-vue is just a "reskin" The main focus should be on VUE not REACT.js RADIX...

One of the beautiful things about vue is that v-model is a standard pattern to bind, why should we break it because of a completely unrelated framework...

<script setup lang="ts">
import { type HTMLAttributes, computed } from 'vue'
import type { CheckboxRootEmits, CheckboxRootProps } from 'radix-vue'
import { CheckboxIndicator, CheckboxRoot, useForwardPropsEmits } from 'radix-vue'
import { CheckIcon } from '@radix-icons/vue'
import { cn } from '@/lib/utils'

const props = defineProps<CheckboxRootProps & { class?: HTMLAttributes['class'] } & { modelValue?: boolean }>()
const emits = defineEmits<CheckboxRootEmits & { 'update:modelValue': [value: boolean]; }>()

const model = defineModel()

const delegatedProps = computed(() => {
  const { class: _, ...delegated } = props

  return delegated
})

const forwarded = useForwardPropsEmits(delegatedProps, emits)
</script>

<template>
  <CheckboxRoot v-bind="forwarded" :checked="props.modelValue"
    @update:checked="(value) => emits('update:modelValue', value)" :class="cn('peer h-4 w-4 shrink-0 rounded-sm border border-primary shadow focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:bg-primary data-[state=checked]:text-primary-foreground',
    props.class)">
    <CheckboxIndicator class="flex h-full w-full items-center justify-center text-current">
      <slot>
        <CheckIcon class="h-4 w-4" />
      </slot>
    </CheckboxIndicator>
  </CheckboxRoot>
</template>

The key parts are the type

const props = defineProps<CheckboxRootProps & { class?: HTMLAttributes['class'] } & { modelValue?: boolean }>()
const emits = defineEmits<CheckboxRootEmits & { 'update:modelValue': [value: boolean]; }>()

and emits on the checkbox

<CheckboxRoot  :checked="props.modelValue"
    @update:checked="(value) => emits('update:modelValue', value)">
Saeid-Za commented 1 week ago

This issue also affects other components like Toggle. If we want to expand the functionality of AutoForm, we must ensure that the interface remains consistent (using modelValue in all cases, as it is the default or conventional way). Thank you for the code provided @sugoidesune, but in my opinion, this is just a temporary fix. The correct approach would be to address this within the radix-vue scope without breaking current behavior. This could involve creating a duplicate prop and event, along with a deprecation warning, until we fully remove the old prop/event. @sadeghbarati, what do you think about this issue?

sadeghbarati commented 1 week ago

@Saeid-Za sry for late response

RadixVue is going to be rebranded someday, but we are still inspired by RadixUI

Making v-model:checked to v-model may helps but I'm not sure about it

We should call the man itself @zernonia

zernonia commented 1 week ago

I've been looking around for this convention as part of Radix Vue v2 these few days, and I found out almost all of the major one (PrimeVue, Nuxt UI etc) are all using v-model. Thus I'm keen to refactor to use v-model as well.

This would of course bring some breaking changes, but that's expected as stated in v2 that we are breaking things (minimally) 😁