Skip to content
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

[Bug]: Class name duplicated #35

Closed
zernonia opened this issue Sep 8, 2023 · 8 comments · Fixed by #241
Closed

[Bug]: Class name duplicated #35

zernonia opened this issue Sep 8, 2023 · 8 comments · Fixed by #241

Comments

@zernonia
Copy link
Member

zernonia commented Sep 8, 2023

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)
  1. 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
Copy link
Collaborator

sadeghbarati commented Sep 8, 2023

I'm with $attrs even though it's not typed yet vuejs/core#7444

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

  • One from vue runtime-dom.d.ts
  • And one from our defineProps

see this SFC playground

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

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Sep 14, 2023

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
Copy link
Contributor

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
Copy link
Collaborator

sadeghbarati commented Sep 26, 2023

That Vue SFC playground sometimes acts inconsistently after editing 😭 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
Copy link
Contributor

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
Copy link

Hi, have we finalize the solution yet?

@sadeghbarati

This comment was marked as off-topic.

@olemarius
Copy link
Contributor

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"]').

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants