react-daisyui icon indicating copy to clipboard operation
react-daisyui copied to clipboard

Table.Head won't accept Table.Row to allow pinCols to work

Open AndrewBrough opened this issue 1 year ago • 5 comments

Simple typescript error that won't allow Table.Head to accept a single child. MDN specifies that the table head pattern should look like table > thead > tr > th, but Table.Head expects only an array of children.

Also, the story for pinned columns doesn't actually use Table.Head, it uses raw HTML instead. I'm not sure if there was a reason for this, but I'd expect to be able to use the react components here in the same pattern eg.

<Table>
  <Table.Head>
    <Table.Row>
       <th>Pinned Header</th>
       <th>Header</th>
       <th>Header</th>
       <th>Header</th>
    </Table.Row>
    <Table.Body>
      <Table.Row>
        <td>data</td>
        <td>data</td>
        <td>data</td>
        <td>data</td>
       <Table.Row>
    </Table.Body>
  </Table.Head>
</Table>

^ this doesn't work, you have to replace Table.Head with the plain html thead instead.

AndrewBrough avatar Sep 11 '24 17:09 AndrewBrough

After looking at the Table.Head component I realized Table.Head expects a set of children elements and wraps them all in a

tag and puts each child in a . This seems not to work with the tailwind pinned columns - you need to have the pinned column cell be a th and the rest tds in the thead row - as seen in the PinnedRowsAndPinnedCols story example.

I'd suggest we change Table.Head to not wrap each child in a

so devs can customize this how they need to while still using the components implemented here.

AndrewBrough avatar Sep 12 '24 16:09 AndrewBrough

@AndrewBrough thanks for finding this and bringing it up. Agreed, seems like the newer DaisyUI table features like pinned have outgrown the old opinionated table components here, and they could be pared back down closer to basic HTML.

Feel free to make a PR if you'd like! Otherwise, I can find some time for it this weekend.

benjitrosch avatar Sep 13 '24 13:09 benjitrosch

I'm working on one now - I realize we might not want to break backwards compatibility by not wrapping components by default, so I'm adding a "noCell" prop set to false by default which, when enabled, removes wrapping th/td tags. If you have a better name for that prop I'm open to it, otherwise I'll tag that PR here when I'm done.

AndrewBrough avatar Sep 13 '24 14:09 AndrewBrough

@AndrewBrough thanks for looking into that, I've just merged your PR. This should be a good temporary fix for now without introducing any breaking changes.

I'll keep this issue open for the future when we can do a bigger refactor (which would see breaking changes and a major version up of the library).

benjitrosch avatar Sep 18 '24 16:09 benjitrosch

Thanks for getting this in so quick! Glad my PR was helpful if a bit verbose. Agreed that maybe for a major version we could remove the noCell prop and just always pass td/th to Table.Head.

I can help contribute to this at that time if it's helpful - I'll keep following this thread.

Edit: I'm waiting to see if there's any feedback after this change. I can make a breaking change PR and leave it open in a few days.

AndrewBrough avatar Sep 19 '24 14:09 AndrewBrough