Skip to content

Conversation

@J-Michalek
Copy link
Contributor

@J-Michalek J-Michalek commented May 4, 2025

πŸ”— Linked issue

Resolves: #4059

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

I have added the ui field in items array - this affects all components that currently use an items prop.

This will simplify cases where the user wants to use a specific classes for different parts of an item but does not want to affect other items at the same time.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@pkg-pr-new
Copy link

pkg-pr-new bot commented May 4, 2025

npm i https://pkg.pr.new/@nuxt/ui@4060

commit: 76dffc0

@benjamincanac
Copy link
Member

I agree this is a nice addition! However, this change should be applied to all components with an items prop for consistency 😬

@J-Michalek
Copy link
Contributor Author

@benjamincanac I can work on this. Should I bunch it up into a one big PR or should I create separate PR for each instance?

@benjamincanac
Copy link
Member

Great! No you can do this in this PR 😊

@J-Michalek J-Michalek changed the title feat(stepper): add iconClass prop to stepper items feat(items): add iconClass prop to items May 5, 2025
@benjamincanac benjamincanac changed the title feat(items): add iconClass prop to items feat(components): add iconClass field in items May 5, 2025
@J-Michalek
Copy link
Contributor Author

J-Michalek commented May 5, 2025

Quick checklist to track the progress on this (I will validate it as I go through it):

  • Accordion
  • Tabs
  • Stepper
  • Carousel
  • Breadcrumb
  • CheckboxGroup
  • RadioGroup
  • InputMenu
  • Tree
  • Select
  • SelectMenu
  • NavigationMenu - here we could support different ui fields for NavigationMenuItem and NavigationMenuChildItem but that would required some refactoring of the component, perhaps in another iteration.
  • DropdownMenu
  • ContextMenu
  • CommandPallete

@benjamincanac benjamincanac marked this pull request as draft May 5, 2025 15:44
@J-Michalek
Copy link
Contributor Author

J-Michalek commented May 5, 2025

@benjamincanac Just one more idea, what if we added a ui prop to the item interface that would allow customization of each part of the item but on a per item basis.

i.e. for the Accordion each item could have a ui prop that would contain the slots that affect the item specifically:

<AccordionItem
      v-for="(item, index) in props.items"
      v-slot="{ open }"
      :key="index"
      :value="item.value || String(index)"
      :disabled="item.disabled"
      :class="ui.item({ class: [props.ui?.item, item.ui?.item] })"
    >
      <AccordionHeader as="div" :class="ui.header({ class: [props.ui?.header, item.ui?.header] })">
        <AccordionTrigger :class="ui.trigger({ class: [props.ui?.trigger, item.ui?.trigger], disabled: item.disabled })">
          <slot name="leading" :item="item" :index="index" :open="open">
            <UIcon v-if="item.icon" :name="item.icon" :class="ui.leadingIcon({ class: [props.ui?.leadingIcon, item?.ui?.leadingIcon] })" />
          </slot>

          <span v-if="get(item, props.labelKey as string) || !!slots.default" :class="ui.label({ class: [props.ui?.label, item.ui?.label] })">
            <slot :item="item" :index="index" :open="open">{{ get(item, props.labelKey as string) }}</slot>
          </span>

          <slot name="trailing" :item="item" :index="index" :open="open">
            <UIcon :name="item.trailingIcon || trailingIcon || appConfig.ui.icons.chevronDown" :class="ui.trailingIcon({ class: [props.ui?.trailingIcon, item.ui?.trailingIcon] })" />
          </slot>
        </AccordionTrigger>
      </AccordionHeader>

      <AccordionContent v-if="item.content || !!slots.content || (item.slot && !!slots[item.slot as keyof AccordionSlots<T>]) || !!slots.body || (item.slot && !!slots[`${item.slot}-body` as keyof AccordionSlots<T>])" :class="ui.content({ class: [props.ui?.content, item.ui?.content] })">
        <slot :name="((item.slot || 'content') as keyof AccordionSlots<T>)" :item="(item as Extract<T, { slot: string; }>)" :index="index" :open="open">
          <div :class="ui.body({ class: [props.ui?.body, item.ui?.body] })">
            <slot :name="((item.slot ? `${item.slot}-body`: 'body') as keyof AccordionSlots<T>)" :item="(item as Extract<T, { slot: string; }>)" :index="index" :open="open">
              {{ item.content }}
            </slot>
          </div>
        </slot>
      </AccordionContent>
    </AccordionItem>

I feel like it would be more future proof as everyone might have different needs of per item customization, of course this is more involved solution but I think it might be worth it.

@benjamincanac
Copy link
Member

We could uniformize this as well indeed, I did it in very specific places where I needed to make an example only at the moment: https://github.com/nuxt/ui/blob/v3/src/runtime/components/Select.vue#L243

@J-Michalek
Copy link
Contributor Author

@benjamincanac Here is a quick commit for the accordion component, if you could quickly have a look if I'm going in the right direction with this.

@J-Michalek J-Michalek changed the title feat(components): add iconClass field in items feat(components): add ui field in items May 6, 2025
@J-Michalek
Copy link
Contributor Author

My mistake, you got a sharp eyes! Thanks for your reviews btw.

Fixed in feb7ca7

@benjamincanac
Copy link
Member

@J-Michalek I've pushed a few fixes, let me know what you think! Is there a reason you ommitted the CommandPalette component?

['with body slot', { props: { ...props, modelValue: '1' }, slots: { body: () => 'Body slot' } }],
['with custom slot', { props: { ...props, modelValue: '5' }, slots: { custom: () => 'Custom slot' } }],
['with custom body slot', { props: { ...props, modelValue: '5' }, slots: { 'custom-body': () => 'Custom body slot' } }]
['with custom body slot', { props: { ...props, modelValue: '5' }, slots: { 'custom-body': () => 'Custom body slot' } }],
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it's a good idea to test everything item.ui here, it's a lot and will be a pain to maintain πŸ˜…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was unsure if I should add test cases for it, I guess we can remove it it is easily the largest block of test code for all of the affected component.

Jakub added 2 commits May 13, 2025 17:02
@J-Michalek
Copy link
Contributor Author

J-Michalek commented May 13, 2025

Thanks for the fixes, seems I was pretty sloppy πŸ™„ I've implemented item.ui and item.class for CommandPalette and removed the tests for item.ui and item.class.

@benjamincanac
Copy link
Member

Not at all! You did an amazing job, thanks a lot 😊

@benjamincanac benjamincanac merged commit b9adc83 into nuxt:v3 May 13, 2025
6 checks passed
benjamincanac added a commit that referenced this pull request May 14, 2025
@benjamincanac
Copy link
Member

benjamincanac commented May 14, 2025

@J-Michalek I've made a mistake regarding #4060 (comment) and reverted it here: 0905b2b.

My argument was that for consistency the item.class should pair with ui.item but as I was refactoring @nuxt/ui-pro to follow these changes, it became clear that the consistency is to target the same elements. For example, with this revert the item.class will be on ui.link but target the LinkBase, like on the DropdownMenu: https://github.com/nuxt/ui/blob/v3/src/runtime/components/DropdownMenuContent.vue#L175 and all other components.

Let me know if I forgot some other places.

benjamincanac added a commit that referenced this pull request May 14, 2025
@J-Michalek
Copy link
Contributor Author

Looks good, I checked my commits that made the original breaking changes and they only affected the Breadcrumb and NavigationMenu components which you both reverted correctly.

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

Labels

v3 #1289

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stepper: Add class for each step's icon

2 participants