unovue / shadcn-vue

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

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

Open sugoidesune opened 5 months ago

sugoidesune commented 5 months 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 5 months ago

See 121

sugoidesune commented 5 months 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 5 months 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 5 months 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 5 months 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) 😁

sugoidesune commented 2 months ago

After quite a few bugs i decided to not use the Checkbox component, and (quickly) made a similar looking "Vanilla" one. It directly passes v-model to the input so it will work with anything. bool, arrays, arrays of objects etc. Since the underlying issue is radix-vue. https://github.com/radix-vue/radix-vue/issues/973

<script setup>
const model = defineModel();
const props = defineProps(["id", "value", "class"]);
</script>

<template>
  <div :class="props.class">
    <label
      class="relative flex cursor-pointer items-center rounded-full"
      :for="props.id"
      data-ripple-dark="true"
    >
      <input
        type="checkbox"
        v-model="model"
        :id="props.id"
        :value="props.value"
        class="peer relative h-4 w-4 cursor-pointer appearance-none rounded border border-gray-800 transition-all checked:bg-slate-900"
      />
      <div
        class="pointer-events-none absolute top-2/4 left-2/4 -translate-y-2/4 -translate-x-2/4 text-white opacity-0 transition-opacity peer-checked:opacity-100"
      >
        <svg
          xmlns="http://www.w3.org/2000/svg"
          class="h-3 w-3"
          viewBox="0 0 20 20"
          fill="currentColor"
          stroke="currentColor"
          stroke-width="0.3"
        >
          <path
            fill-rule="evenodd"
            d="M16.707 5.293a1 1 0 010 1.414l-8 8a1 1 0 01-1.414 0l-4-4a1 1 0 011.414-1.414L8 12.586l7.293-7.293a1 1 0 011.414 0z"
            clip-rule="evenodd"
          ></path>
        </svg>
      </div>
    </label>
  </div>
</template>
DesertCookie commented 1 month ago

After quite a few bugs i decided to not use the Checkbox component, and (quickly) made a similar looking "Vanilla" one. [...]

Will be kindly using your component for the time being. Thanks a lot.