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

feat: make tr loading classes configurable #1756

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hempsworth
Copy link

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

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

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

@hempsworth hempsworth marked this pull request as draft July 1, 2024 21:42
@hempsworth hempsworth marked this pull request as ready for review July 1, 2024 21:43
@lrljoe
Copy link
Sponsor Collaborator

lrljoe commented Jul 1, 2024

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
Copy link
Sponsor Collaborator

lrljoe commented Jul 1, 2024

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!

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