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

refactor: use class as prop to prevent class duplication with cn function #241

Merged
merged 28 commits into from
Feb 2, 2024

Conversation

sadeghbarati
Copy link
Collaborator

@sadeghbarati sadeghbarati commented Jan 3, 2024

close #35
close #310

This might take a bit longer cause some of the classes/styles changed in main shadcn-ui so I have to bring them in here too


Previously we used destructure useAttrs like 👇 , to fix class duplication

defineOption({
  inheritAttrs: false
})

const { class: className, ...rest } = useAttrs()

But destructure props or attrs will cause lose reactivity

We could make attrs reactive with computed or toRefs,
But making Always read-only (inside the component) thing, reactive/mutable doesn't make sense

image


Now we are using class in prop and delegated/rest

delegated/rest spread props

<script setup lang="ts">
import { type HTMLAttributes, computed } from 'vue'
import { AccordionContent, type AccordionContentProps } from 'radix-vue'
import { cn } from '@/lib/utils'

const props = defineProps<AccordionContentProps & { class?: HTMLAttributes['class'] }>()

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

  return delegated
})
</script>

<template>
  <AccordionContent
    v-bind="delegatedProps"
    :class="
      cn(
        'overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down',
        props.class,
      )
    "
  >
    <div class="pb-4 pt-0">
      <slot />
    </div>
  </AccordionContent>
</template>


  • After this rfcs [Core Team RFC] Reactive Props Destructure vuejs/rfcs#502 comes out of experimental we will have this code

    Details

    <script setup lang="ts">
    import type { HTMLAttributes } from 'vue'
    import { AccordionContent, type AccordionContentProps } from 'radix-vue'
    import { cn } from '@/lib/utils'
    
    const { class: className, ...delegated  } = defineProps<AccordionContentProps & { class?: HTMLAttributes['class'] }>()
    </script>
    
    <template>
      <AccordionContent
        v-bind="delegated "
        :class="
          cn(
            'overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down',
            className,
          )
        "
      >
        <div class="pb-4 pt-0">
          <slot />
        </div>
      </AccordionContent>
    </template>

  • And after complex types support for defineProps, we will have this, hopefully xd

    Details

    <script setup lang="ts">
    import type { HTMLAttributes } from 'vue'
    import { AccordionContent, type AccordionContentProps } from 'radix-vue'
    import { cn } from '@/lib/utils'
    
    const { class: className, ...delegated  } = defineProps<AccordionContentProps & HTMLAttributes>()
    </script>
    
    <template>
      <AccordionContent
        v-bind="delegated "
        :class="
          cn(
            'overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down',
            className,
          )
        "
      >
        <div class="pb-4 pt-0">
          <slot />
        </div>
      </AccordionContent>
    </template>

  • Complex types + generics (not real code 😂) for example take look at this react article

https://www.totaltypescript.com/pass-component-as-prop-react

Details

<script setup lang="ts" generic="T extends AsTag">
import type { IntrinsicElementAttributes } from 'vue'
import { AccordionContent, type AccordionContentProps } from 'radix-vue'
import { cn } from '@/lib/utils'

const { class: className, ...delegated  } = defineProps<AccordionContentProps & IntrinsicElementAttributes[T]>()
</script>

<template>
  <AccordionContent
    v-bind="delegated "
    :class="
      cn(
        'overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down',
        className,
      )
    "
  >
    <div class="pb-4 pt-0">
      <slot />
    </div>
  </AccordionContent>
</template>

@sadeghbarati sadeghbarati changed the title refactor: use class as prop to prevent class duplication refactor: use class as prop to prevent class duplication with cn function Jan 18, 2024
@sadeghbarati sadeghbarati marked this pull request as ready for review January 20, 2024 20:13
@sadeghbarati
Copy link
Collaborator Author

sadeghbarati commented Jan 20, 2024

This PR Depends on this change 👇 in radix-vue to be released

radix-vue/radix-vue#628


Please remove the commit body if you want to Merge it into dev 🤣

Details

image

@sadeghbarati

This comment was marked as resolved.

@sadeghbarati

This comment was marked as resolved.

@sadeghbarati

This comment was marked as resolved.

import { AccordionContent, type AccordionContentProps } from 'radix-vue'
import { cn } from '@/lib/utils'

const props = defineProps<AccordionContentProps & { class?: string }>()
const props = defineProps<AccordionContentProps & { class?: HTMLAttributes['class'] }>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry this HTMLAttributes would cause some issue for detypes 😂

@zernonia
Copy link
Member

zernonia commented Feb 2, 2024

@sadeghbarati do upgrade the radix-vue deps to v1.4.0

@sadeghbarati sadeghbarati merged commit a829212 into radix-vue:dev Feb 2, 2024
1 check passed
@sadeghbarati sadeghbarati deleted the refactor/use-class-prop branch February 2, 2024 15:38
@hendrasan
Copy link

hendrasan commented Feb 13, 2024

I'm not sure whether this is the right place or not, but i found some bugs that I traced back to this commit.

I'm using vue 3.3.8, haven't upgraded to vue 3.4 yet.

So in SelectItem.vue, the problem is in this code

const props = defineProps<SelectItemProps & { class?: HTMLAttributes['class'] }>()

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

  return delegated
})

const forwardedProps = useForwardProps(delegatedProps)
console.log('f', forwardedProps)

If I'm using radix-vue version 1.2.3, this breaks as the type of forwardedProps become CustomRefImpl when I console.log it, and the value of it becomes an object like this:

value: {
    "value": "pineapple",
    "disabled": false,
    "asChild": false
}

To get around this, I tested two solutions:

  1. Upgrade radix-vue to 1.4.0, then the type of forwardedProps become the expected ComputedRefImpl and the value is the expected string value, which is "pineapple" in the above example. But in radix-vue changelog, it said that v1.3.0 required vue 3.4 (not sure if it's just for the treeshaking feature, I tested radix-vue 1.4.0 in this vue 3.3.8 app and it seemed to still work fine, haven't tested the whole app)
  2. Stay in radix-vue 1.2.3, don't use the forwardedProps, but directly use the props to pass to v-bind in SelectItem component, not sure if this is a good workaround though.

Also, whenever i add a component using npx shadcn-vue@latest add select, it prompts me to upgrade the shadcn-vue version, but IIRC the radix-vue package is installed only during npx shadcn-vue@latest init. So if there are changes that require updates to radix-vue like this one, do I have to manually install the updated radix-vue or can it be automated in some way?

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

Successfully merging this pull request may close these issues.

[Bug]: Uncontrolled collapsible not working [Bug]: Class name duplicated
3 participants