spartan icon indicating copy to clipboard operation
spartan copied to clipboard

Published 20 hours ago • verified publisher icon goetzrobin

refactor(table): rewrite `Table`

Open alexciesielski opened this issue 1 year ago • 3 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] The commit message follows our guidelines: https://github.com/goetzrobin/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
  • [ ] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [x] Bugfix
  • [x] Feature
  • [ ] Code style update (formatting, local variables)
  • [x] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Documentation content changes
  • [ ] Other... Please describe:

Which package are you modifying?

  • [ ] accordion
  • [ ] alert
  • [ ] alert-dialog
  • [ ] aspect-ratio
  • [ ] avatar
  • [ ] badge
  • [ ] button
  • [ ] calendar
  • [ ] card
  • [ ] checkbox
  • [ ] collapsible
  • [ ] combobox
  • [ ] command
  • [ ] context-menu
  • [ ] data-table
  • [ ] date-picker
  • [ ] dialog
  • [ ] dropdown-menu
  • [ ] hover-card
  • [ ] input
  • [ ] label
  • [ ] menubar
  • [ ] navigation-menu
  • [ ] popover
  • [ ] progress
  • [ ] radio-group
  • [ ] scroll-area
  • [ ] select
  • [ ] separator
  • [ ] sheet
  • [ ] skeleton
  • [ ] slider
  • [ ] switch
  • [x] table
  • [ ] tabs
  • [ ] textarea
  • [ ] toast
  • [ ] toggle
  • [ ] tooltip
  • [ ] typography

What is the current behavior?

Context can be found in Discord

What is the new behavior?

Does this PR introduce a breaking change?

  • [x] Yes
  • [ ] No

Other information

As discussed in Discord, this fixes the issue of dynamically rendering columns using @for which caused errors in the underlying CdkTable.

This is basically a complete rewrite of spartan's table by basically copying Angular Material's implementation and replacing the mat prefixes with brn and hlm in their respective places and applying proper classes in the hlm parts. I hope I didn't miss any classes.

Usage is exactly like Angular Material's table.

One thing I'm not sure on is the mixed usage of brn and hlm components in the final consuming code. My preference would be to have a HlmTableComponent that simply extends the BrnTableComponent and just applies the same template and providers in its @Component decorator plus some Tailwind classes, as I implemented - but I'm open to your suggestions

And in regards to the documentation I was thinking of copying parts of Materials table docs and for the example code applying on of the stories' code?

Left to do:

  • [x] update Storybook
  • [ ] styling for child components / ability to set/override tailwind classes
  • [ ] update documentation text
  • [x] update documentation example code
  • [ ] implement caption component
  • [ ] fix pagination
  • [ ] adjust generator .template files

alexciesielski avatar Feb 04 '24 08:02 alexciesielski

I'll try to find some time this weekend to look at this!

goetzrobin avatar Feb 16 '24 17:02 goetzrobin

Is there any progress on this? I am willing to help if someone lets me know how :)

martinledl avatar Jul 10 '24 12:07 martinledl

@martinledl unfortunately I haven't had the bandwidth to give this the attention it deserved. I would like to see if we can keep the current API surface to an extend and not necessarily force everyone to work at the same low level as the cdk. However, if the current implementation is too limiting I am open to revisit this material like approach

goetzrobin avatar Jul 10 '24 14:07 goetzrobin