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

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

Open
AndrewBrough opened this issue Sep 11, 2024 · 5 comments
Open

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

AndrewBrough opened this issue Sep 11, 2024 · 5 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@AndrewBrough
Copy link
Contributor

AndrewBrough commented Sep 11, 2024

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

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.

@benjitrosch
Copy link
Collaborator

@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.

@AndrewBrough
Copy link
Contributor Author

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.

@benjitrosch
Copy link
Collaborator

@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 benjitrosch added enhancement New feature or request help wanted Extra attention is needed labels Sep 18, 2024
@AndrewBrough
Copy link
Contributor Author

AndrewBrough commented Sep 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants