unovue / shadcn-vue

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

[Bug]: Class name duplicated #35

Closed zernonia closed 8 months ago

zernonia commented 1 year ago

If you inspecting some element, you would notice a duplicated class name mainly due to using 2. approach mentioned below.

For components in registry, you would see 2 different approach for passing the class name into cn function.

  1. using class props.

    const props = defineProps<... & { class?: string }>()
    ...
    cn(...., props.class)
  2. using inherited attributes

    cn(..., $attrs.class)

The reason I'm hesitating is because I don't like to declare class props in every component, using $attrs.class seems much more intuitive. wdyt??

sadeghbarati commented 1 year ago

I'm with $attrs even though it's not typed yet https://github.com/vuejs/core/pull/7444

However if we extend Props based on HTMLAttributes like shadcn, we will have two class prop when we hit Ctrl + Space

see this SFC playground

Edit: external types error and vue props destructuring error with class keyword in Button.vue in SFC playground

sadeghbarati commented 1 year ago

Base on so1ve answer in Vue issue (How to import interface for defineProps)

imported external interface should not be extended so Vue can resolve the type

We should have an interface like this

- export interface ButtonHTMLAttributes extends HTMLAttributes {
+ export interface OnlyButtonHTMLAttributes {
  autofocus?: Booleanish
  disabled?: Booleanish
  form?: string
  formaction?: string
  formenctype?: string
  formmethod?: string
  formnovalidate?: Booleanish
  formtarget?: string
  name?: string
  type?: 'submit' | 'reset' | 'button'
  value?: string | string[] | number
}

https://github.com/wooorm/html-element-attributes/blob/main/index.js

libondev commented 1 year ago

Because the default value of the inheritAttrs attribute in the vue component is true, I think the processing of $attrs.class can be ignored in components that are not explicitly set to inheritAttrs: false😃

sadeghbarati commented 1 year ago

That Vue SFC playground sometimes acts inconsistently after editing :sob: for example, it's merge classes even if you use $attrs.class and v-bind="$attrs"

I think we should use useAttrs to destructure the object to access the class key

New SFC playground


useAttrs (not reactive when destructered and make them reactive with computed or other Vue apis is not good choice cause attrs are readonly proxy)

Component

<script setup lang="ts">
import { useAttrs } from 'vue'
import { cn } from '@/lib/utils'

defineOptions({
  inheritAttrs: false,
})

const { class: className, ...rest } = useAttrs() // not reactive only works on first render
</script>

<template>
  <input type="text" :class="cn('flex h-9 w-full text-green-600', className ?? '')" v-bind="rest">
</template>

Usage


<script setup lang="ts">
import Input from './Input.vue'
</script>

<template>
    <Input class="text-red-600"/> <!-- class output: "flex h-9 w-full text-red-600" -->
</template>
libondev commented 1 year ago

That Vue SFC playground sometime acts inconsistently 😭

<script setup lang="ts">
import { useAttrs } from 'vue'
import { cn } from '@/lib/utils'

defineOptions({
  inheritAttrs: false,
})

const { class: className, ...rest } = useAttrs()
</script>

<template>
  <input type="text" :class="cn('flex h-9 w-full', className ?? '')" v-bind="rest">
</template>

Let's try this 👀 https://play.vuejs.org/#eNp9Uk1PIzEM/StRtNIUiU53xa07uxK74gAHQMAxlyHjaVNSJ0o8pQj1v2OnHxQJ0GiixM/v5dnxqz6PsV4NoKe6yTa5SCoDDVH5Fmd/jKZs9F+DbhlDIvUAmf4H3iMgqT6FparqiUREo/ptUL5mslViHh8IltG3BHxSqvmoYH2bM9/SxgjYjcvRaDXh3GZyIOpTtmED9m5WL3JA9voqYkZbVnIe0k0kF5CpU1UQwVrvw/NViVEa4HQft3OwT5/EF3ktMaNvE2RIKzD6gFGbZkBb+OL+Gta8P4DL0A2es78B7yAHP4jHbdq/ATu2fZRX3F6WLjucPeSLNQHmfVFiVDI3Jd9obrY08avS3+2e1WeFZ3DDXdw/1PePnQz2A1qRVRZHdV1bn09YnQHRT0xJDPlcL4LDUaWqE8E2ssjfQe8QdsZGO57DOSRH50QpT1Xf+sytF1bhvo/MTuNobAq9cRgHUvQSQYyWJqvpfn7YZtV7WKv5+NdPdqPVavzosGPoRys37uo6HqrNGxWWArM=

binhntfiber commented 1 year ago

Hi, have we finalize the solution yet?

sadeghbarati commented 1 year ago

Hi, have we finalize the solution yet?

I can refactor all components to useAttrs and after Vue 3.4 to defineAttrs But before that, I need approval from Vue wizards :raised_hands: @ zernonia

olemarius commented 11 months ago

Note that setting inheritAttrs: false makes it harder to use e.g. Cypress. The recommended way to select elements for a test is by using data-cy="my-element", then getting it with cy.get('[data-cy="my-element"]').