vuetifyjs / vuetify

🐉 Vue Component Framework
https://vuetifyjs.com
MIT License
39.81k stars 6.96k forks source link

[Bug Report][3.4.6] VDataTable - Using #thead and #tbody slots does not remove VDataTable thead or tbody #18854

Closed MarcHartwig13 closed 5 months ago

MarcHartwig13 commented 10 months ago

Environment

Vuetify Version: 3.4.6 Vue Version: 3.3.9 Browsers: Firefox 120.0 OS: Mac OS 10.15

Steps to reproduce

Add a #thead slot with the VDataTable component. Notice the thead element for the headers remains.

Add a #tbody slot to VDataTable and notice the tbody with the data remains.

Expected Behavior

When using the #thead or #tbody slot I'd expect this to completely replace the <thead> and <tbody> of the VDataTable for when I want to implement my own <thead> and <tbody> elements

Actual Behavior

When adding a #thead or #tbody slot the content within that slot is added as a sibling to the VDataTable <thead> and <tbody> element.

Reproduction Link

https://play.vuetifyjs.com/#...

MarcHartwig13 commented 10 months ago

The thead and tbody slots are documented in the slots section of the page you linked:

thead: https://vuetifyjs.com/en/api/v-data-table/#slots-thead

tbody: https://vuetifyjs.com/en/api/v-data-table/#slots-tbody

Also using the headers slot does not allow me to replace the thead element. I'd like to add props / styling / classes / etc onto the thead. Example: Can't provide own thead

craigrileyuk commented 7 months ago

I was just about to post this issue. Seems like the issue is here:

// vuetify/packages/vuetify/src/components/VDataTable/VDataTable.tsx
default: () => slots.default ? slots.default(slotProps.value) : (
    <>
      { slots.colgroup?.(slotProps.value) }
      <thead>
        <VDataTableHeaders
          { ...dataTableHeadersProps }
          v-slots={ slots }
        />
      </thead>
      { slots.thead?.(slotProps.value) }
      <tbody>
        { slots['body.prepend']?.(slotProps.value) }
        { slots.body ? slots.body(slotProps.value) : (
          <VDataTableRows
            { ...attrs }
            { ...dataTableRowsProps }
            items={ paginatedItems.value }
            v-slots={ slots }
          />
        )}
        { slots['body.append']?.(slotProps.value) }
      </tbody>
      { slots.tbody?.(slotProps.value) }
      { slots.tfoot?.(slotProps.value) }
    </>
  ),

The body slot isn't rendered until we're already inside the element, so it doesn't replace it.

codetakki commented 7 months ago

Im would like to replace my tbody with vuedraggable, it worked in vuetify 2 as the body slot replaced the default tbody, but now it dose not. Unless I'm missing something what i would like to see working:

  <v-data-table
      :headers="headers"
      :items="items"
      :items-per-page="-1"
    >
      <template #body="{ items }">
        <draggable tag="tbody" :list="items">
          <tr
            v-for="item in items"
            :key="item.name"
          >
            ...
          </tr>
        </draggable>
      </template>
    </v-data-table>

Currently the vuetify styling is broken resulting in a ugly list: image

Amebus commented 7 months ago

@codetakki I had the same issue, tight now the only workaround I was able to achieve is something like this

<template 
    v-if="allowItemsDragAndDrop"
   #body
   >
<!-- workaround for the following issue https://github.com/vuetifyjs/vuetify/issues/18854 -->
</template>
<template
  v-if="allowItemsDragAndDrop"
  #tbody="bodySlotProps"
  >
 <!-- my tbody here-->
 </template>
webdevnerdstuff commented 6 months ago

The thead and tbody slots are just to add those elements, it's not a replacement. headers is the slot to completely replace the initial thead and body is the one to replace the initial tbody. There are instances where someone might want multiple thead or tbody and even tfoot, and do not want to remove the initial ones.

Personally I wouldn't call this a bug, but more like the documentation needs info for the MISSING DESCRIPTION.

WillowWisp commented 6 months ago

The thead and tbody slots are just to add those elements, it's not a replacement. headers is the slot to completely replace the initial thead and body is the one to replace the initial tbody. There are instances where someone might want multiple thead or tbody and even tfoot, and do not want to remove the initial ones.

Personally I wouldn't call this a bug, but more like the documentation needs info for the MISSING DESCRIPTION.

Except that the body slot replace the children of the tbody instead of replacing the tbody tag itself (like in the previous version). So it seems to me that either body or tbody slot is bugged since now there's no way to replace the whole defaault tbody tag, right?

webdevnerdstuff commented 6 months ago

Except that the body slot replace the children of the tbody instead of replacing the tbody tag itself (like in the previous version). So it seems to me that either body or tbody slot is bugged since now there's no way to replace the whole defaault tbody tag, right?

Valid point that the body slot doesn't replace the tbody element. TBH, I've never had the need to completely replace the tbody element as I've never had use case for it, and I use data tables extensively. But to the OP's post, the thead and tbody slots I believe are working as intended and are not bugs. Those make perfect sense to me personally.

To fix body replacing tbody would be a breaking change for people who do use the body slot already. I looked at the source code and it's probably not too hard to fix. @nekosaur was this the intended functionality? I can probably fix it if it was not, but might need to wait for a minor version change?

craigrileyuk commented 5 months ago

TBH, I've never had the need to completely replace the tbody element as I've never had use case for it, and I use data tables extensively.

This is not a helpful response. There's very obviously a very common use case and one that's been highlighted by multiple people in this thread.

You need access to the tbody element to use any sorting libraries, like say, VueUse's useSortable composable. Having the need to sort items in a data table is extremely common.

codetakki commented 5 months ago

Agreed with @craigrileyuk, as i mentioned, the only way to use libraries that really on controlling their own list items by iteration is to replace the tbody tag. And its definitely not logical how the tbody and body slots are used currently, check out #19413 To why. The tbody slot is placed in the same level as tbody, and body slot inside. To me this makes no sense that the tbody slot appends a tbody to the table. If you wanted to have multiple tbodys i think one should be able to do so by replacing the original tbody as well as adding additional tbody tags. (ps) Feel free to interact with my pr to fix this, so we can edit it to match most needs

webdevnerdstuff commented 5 months ago

TBH, I've never had the need to completely replace the tbody element as I've never had use case for it, and I use data tables extensively.

This is not a helpful response. There's very obviously a very common use case and one that's been highlighted by multiple people in this thread.

You need access to the tbody element to use any sorting libraries, like say, VueUse's useSortable composable. Having the need to sort items in a data table is extremely common.

Why would someone use a sorting library like VueUse useSortable, when that is a built in functionality of the VDataTable along with it's headers? There are plenty of ways to sort a VDataTable with or without a sorting library. That is not a common use case, as that defeats the purpose of the VDataTable component. What that's doing is basically trying to turn the VDataTable into a VTable, so you can then turn it back into a VDataTable, aka redundant and completely unneeded. If you would like to change my mind, show me a working example in the Vuetify Playground on how it's "not possible" to do what you are trying to do. Or post it in the Vuetify Discord or StackOverflow and other can help you.

Other cases people are posing also make it sound like people should be using either the default slot, or the VTable component instead of the VDataTable as you are asking to change (and introduce breaking changes) to how the VDataTable works. As for "sorting", that just sounds like you don't really understand how the VDataTable works, especially if your argument is about sorting rows.

If you don't know how to do something, then perhaps ask StackOverflow, or join the Vuetify Discord channel where people can also get help from people who know how to use Vuetify extensively.

codetakki commented 5 months ago

TBH, I've never had the need to completely replace the tbody element as I've never had use case for it, and I use data tables extensively.

This is not a helpful response. There's very obviously a very common use case and one that's been highlighted by multiple people in this thread.

You need access to the tbody element to use any sorting libraries, like say, VueUse's useSortable composable. Having the need to sort items in a data table is extremely common.

Why would someone use a sorting library like VueUse useSortable, when that is a built in functionality of the VDataTable along with it's headers? There are plenty of ways to sort a VDataTable with or without a sorting library. That is not a common use case, as that defeats the purpose of the VDataTable component. What that's doing is basically trying to turn the VDataTable into a VTable, so you can then turn it back into a VDataTable, aka redundant and completely unneeded. If you would like to change my mind, show me a working example in the Vuetify Playground on how it's "not possible" to do what you are trying to do. Or post it in the Vuetify Discord or StackOverflow and other can help you.

Other cases people are posing also make it sound like people should be using either the default slot, or the VTable component instead of the VDataTable as you are asking to change (and introduce breaking changes) to how the VDataTable works. As for "sorting", that just sounds like you don't really understand how the VDataTable works, especially if your argument is about sorting rows.

If you don't know how to do something, then perhaps ask StackOverflow, or join the Vuetify Discord channel where people can also get help from people who know how to use Vuetify extensively.

In these cases "sortable" refers to being able to drag items in the list around, enabling resorting of the list. If you ask stack overflow you will get the answer to use the #body slot for this, witch works for vuetify 2 https://stackoverflow.com/questions/66090612/can-vue-draggable-be-used-with-vuetify-v-data-table-and-allow-utilisation-of-tab#comment135280475_66090613

MajesticPotatoe commented 5 months ago

To me, logically, tbody/thead etc etc should replace the content that the slot is depicted by. However, as stated https://github.com/vuetifyjs/vuetify/issues/17206#issuecomment-1529603656 They were initially intended as additions to the slot, to not have to reinvent default functionality (as thats what slots tend to do, which has its use itself).

Personally I think that logically the naming should be tbody to handle the whole tbody (etc etc), and then have a prepend/append slot for the additions as that would be consistent with how other slots work. However technically these would be breaking changes.

Current structure is like this

table
  top slot
  default slot
    thead (removable with hide-default-header prop)
    thead slot
    tbody (Propose a hide-default-body prop)
      body.prepend slot
      body slot
      body.append slot
     tbody slot
   bottom slot

With that being said, for v3, may just need a better documentation to outline it, and we can add a hide-default-body prop (which would make it as a feature), that way, it would at least mimic the slot functionality without completely removing if needed, and can consider a minor breaking restructure for v4.

This would make it so doing

<v-data-table hide-default-body>
  <template #tbody />
</v-data-table>

which would render just your one custom tbody, but allow someone to just append if needed

webdevnerdstuff commented 5 months ago

In these cases "sortable" refers to being able to drag items in the list around, enabling resorting of the list. If you ask stack overflow you will get the answer to use the #body slot for this, witch works for vuetify 2 https://stackoverflow.com/questions/66090612/can-vue-draggable-be-used-with-vuetify-v-data-table-and-allow-utilisation-of-tab#comment135280475_66090613

You didn't mention draggable when you mentioned sorting in that comment. I missed that you posted about draggable and thought it was someone else, so I didn't think you were talking about the same thing. It makes more sense now that you made that clear.

As I mentioned before, and as MajesticPotatoe also mentioned, while it is a valid point, it's still a breaking change to completely change how it works. This is why I don't consider it a "bug" but more of a "feature request" as it currently works as intended. A hide-default-body prop would be a decent solution and easy enough to implement imo. But I do think the thead slot is fine as is.

MajesticPotatoe commented 5 months ago

As of v3.6.5 thead slot works in requested fashion in conjunction with hide-default-header prop like so:

<v-data-table hide-default-header>
  <template #thead>
    <thead />
  </template>
</v-data-table>

I will be putting in a fix in for the next patch to have the same functionality using tbody slot + a hide-default-body prop eg:

<v-data-table hide-default-body>
  <template #tbody>
    <tbody />
  </template>
</v-data-table>

This will suffice till v4 when we can revisit slot names + structure and introduce a breaking change.