rappasoft / laravel-livewire-tables

A dynamic table component for Laravel Livewire
https://rappasoft.com/docs/laravel-livewire-tables/v2/introduction
MIT License
1.7k stars 320 forks source link

feat: make tr loading classes configurable #1756

Open hempsworth opened 3 days ago

hempsworth commented 3 days ago

This adds the ability to configure the classes used when a tr is in a loading state, making it easier to theme.

All Submissions:

New Feature Submissions:

  1. [x] Does your submission pass tests and did you add any new tests needed for your feature?
  2. [x] Did you update all templates (if applicable)?
  3. [x] Did you add the relevant documentation (if applicable)?
  4. [x] Did you test locally to make sure your feature works as intended?
lrljoe commented 3 days ago

Thanks for the commit!! Love it when people add to the project!

However, I can see already there'll be a couple of merge conflicts, as there's some pending release items in "development", which are touching the same files (namely, a change to hover behaviour in bootstrap - #1742 ).

If you can please spin up a fresh branch from "development", and add the same changes in, then I can run the tests and merge it across.

I can see you've written a doc, which is fantastic, could you please also add in a basic test? It can be as simple as a "set", then retrieve the value, set it again, retrieve it, checking that it matches expected each time.

We also tend to add a "has" method to determine if something is set or not, rather than using isset() in the blades themselves, just so that it's all centralised and easy to maintain, as people do tend to publish the views, and then miss out on changes.

Also, we use the merge functionality, rather than appending, just to keep things standardised, not sure off the top of my head if this would apply here tho!

Final thing I'd probably add, is I'm trying (slowly) to add in two "default" options rather than just the one for anything new, i.e: default-styling default-colors

For example, "pagination-dropdown"

        {{ 
            $attributes->merge($component->getPerPageFieldAttributes())
            ->class([
                'form-control' => $component->isBootstrap4() && $component->getPerPageFieldAttributes()['default-styling'],
                'form-select' => $component->isBootstrap5() && $component->getPerPageFieldAttributes()['default-styling'],
                'block w-full rounded-md shadow-sm transition duration-150 ease-in-out sm:text-sm sm:leading-5 focus:ring focus:ring-opacity-50' => $component->isTailwind() && $component->getPerPageFieldAttributes()['default-styling'],
                'border-gray-300 focus:border-indigo-300 focus:ring-indigo-200 dark:bg-gray-700 dark:text-white dark:border-gray-600' => $component->isTailwind() && $component->getPerPageFieldAttributes()['default-colors'],
            ])
            ->except(['default','default-styling','default-colors']) 
        }}

It's not a big deal for this one given how few classes are on the item(s), it's more for when there's a lot of different classes being set.

Feel free to give me a poke on the Discord if you need pointers on anything above, or want to discuss

lrljoe commented 3 days ago

Just to add, I'm in two minds as to whether this even needs to be a callable, or if it can just be something defined more globally.

I highly doubt that anyone would ever want or need to change the classes of the "loading" appearance based on anything that is in the row.

So it may be simpler/more performant to avoid the callable element, and just define a set of classes, that will apply on all rows when loading. There's no issue with having both approaches though!