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

refactor(table): rewrite Table #145

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

alexciesielski
Copy link
Contributor

@alexciesielski alexciesielski commented Feb 4, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • 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
  • 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?

  • 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:

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

@goetzrobin
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants