[Bug]: Problems with clean published views

shawe commented 2 weeks ago

What happened?

I'm having an issue caused by bg-light on tr, that is causing a white color on my dark theme.

But another problem appears if I try to fix it removing this class from your published views. The problem appears only for use the published views, without any modification.

How to reproduce the bug

I run: php artisan vendor:publish --provider="Rappasoft\LaravelLivewireTables\LaravelLivewireTablesServiceProvider" --tag=livewire-tables-views to customize it, but before do any change I try the order action by column.

When I don't have the published views, all is working. But when the view files are published (instead without changes), then problems are appearing only to try reorder by a column.

Package Version

3.4.17

PHP Version

8.2.x

Laravel Version

11.21.0

Alpine Version

Default version included with livewire 3.5.6

Theme

Bootstrap 5.x

Notes

I really don't require to modify the views, but the style classes used in your views are hard-coded on views. Maybe was a better solution to have it on the configuration file, that can allow to all people also set a diferent default style without touching the views.

Error Message

No response

lrljoe commented 1 week ago

@shawe

Which classes are you talking about?

There are numerous methods to replace the default classes For example setThAttributes, setTdAttributes, setTrAttributes etc

It's explained in the styling sections on the docs site.

Is there a specific one that you're wanting to customise that doesn't exist at the moment?

Replacing the default tr classes is simple without publishing the views

lrljoe commented 1 week ago

For example (from the docs site)

setTrAttributes

Set a list of attributes to override on the tr elements

public function configure(): void
{
  // Takes a callback that gives you the current row and its index
  $this->setTrAttributes(function($row, $index) {
      if ($index % 2 === 0) {
        return [
          'class' => 'bg-gray-200',
        ];
      }

      return [];
  });
}

By default, this replaces the default classes on the tr, if you would like to keep them, set the default flag to true.

public function configure(): void
{
  $this->setTrAttributes(function($row, $index) {
      if ($index % 2 === 0) {
        return [
          'default' => true,
          'class' => 'bg-gray-200',
        ];
      }

      return ['default' => true];
  });
}

lrljoe commented 1 week ago

Using this approach allows you to customise the classes based on values of columns etc, which is much better than just setting a load of classes in a config file.

For example:

public function configure(): void
{
  // Takes a callback that gives you the current column, row, column index, and row index
  $this->setTdAttributes(function(Column $column, $row, $columnIndex, $rowIndex) {
    if ($column->isField('total') && $row->total < 1000) {
      return [
        'class' => 'bg-red-500 text-white',
      ];
    }
    elseif ($column->isField('total') && $row->total >= 1000) {
      return [
        'class' => 'bg-green-500 text-white',
      ];
    }

    return ['default' => true];
  });
}

shawe commented 1 week ago

@shawe

Which classes are you talking about?

There are numerous methods to replace the default classes For example setThAttributes, setTdAttributes, setTrAttributes etc

It's explained in the styling sections on the docs site.

Is there a specific one that you're wanting to customise that doesn't exist at the moment?

Replacing the default tr classes is simple without publishing the views

With bg-light on tr:

Without bg-light on tr:

Without bg-light with bootstrap-5 theme, it looks like your screenshots here: https://rappasoft.com/docs/laravel-livewire-tables/v3/introduction

I will try now your suggestions to use https://rappasoft.com/docs/laravel-livewire-tables/v3/datatable/styling#content-settrattributes but I think that is not a good idea have a copy&paste portion of code for all tables to be a clean solution (yes could be moved to a trait, or any other place).

I understand that this would be used to have some uncommon styles on some specific places but not manually add to all tables in each use. For this reason I suggest to move the default styles to another place like an specific config file for the styles. There are other ways, you now better than me.

The other important thing here is, why publish the view files (without modify it) would cause it to work differently. Yes I know that it's advised that is not a recommended way, but one thing is not to be the recommended way, and very different is that only for publish some issues start appearing without apparently reason becauses is the original code.

I'm not sure if its an issue with mazer theme or not, but I think not because here https://getbootstrap.com/docs/5.3/content/tables/#striped-rows with dark theme enabled, don't have bg-light and bg-light was used here https://getbootstrap.com/docs/5.3/content/tables/#variants to force colors. For me this is the visual issue, a not needed forced color.

I will use your recommendation to solve it, but I don't believe is the way, because force all devs using this to remember to add this portion of code on all configure for a new table, instead of centralize a solution that reduce and simplify the code. But yes, it's only my opinion, you decide what is the correct way to do things here because you know it better how its designed. I can see that you are assuming that I want to customize the style, but no I believe that I'm confirming that the default style is not good enough.

In my case, this code based in your suggestion seems to be what is expected.

        $this->setTrAttributes(function($row, $index) {
            return [
                'default' => false,
                'class' => 'rappasoft-striped-row',
            ];
        });

EDIT: bg-light and bg-white are not needed, base striped class to the work.

For my specific problem with the style I have a solution with that, but I believe that the other problems related with that would be revised, the breaking code for publish views, and if bg-light is real needed or not.

Thanks for all!

shawe commented 1 week ago

I don't want to create a discussion with that (I see that this was closed before), only leave the details that maybe will help in the future to others if this classes are not changed for bootstrap-5.

Tailwind seems to invert the light/dark color when in light or dark mode, but boostrap don't do this.

The problematic classes with light/dark classes that you are in consideration on tailwind but not in boostrap-5:

https://github.com/rappasoft/laravel-livewire-tables/blob/master/resources/views/components/table/tr.blade.php#L28

https://github.com/rappasoft/laravel-livewire-tables/blob/master/resources/views/components/table/tr.blade.php#L29

Bootstrap samples with light/dark change option on top right:

https://getbootstrap.com/docs/5.3/content/tables/

Overview has a basic sample
Variants has forced colors by tr (like here)
Striped rows

lrljoe commented 6 days ago

@shawe

Can you share:

What you've put in the setTrAttributes
The result (please wrap the code in three ` marks either side so that I can easily grab it and work with it)

Keep in mind that you have the "index", so you can tell if it is even/odd via a

$index % 2 === 0
// or
$index % 2 !== 0

In setTrAttributes

        $this->setTrAttributes(function($row, $index) {
            return [
                'default' => false,
                'class' => 'rappasoft-striped-row',
            ];
        });

The problem with the default bootstrap striped behaviour is:

There is a hidden row for each row on the table, which is used by the Collapse Column feature, as the data is added to that row when the table collapses due to tablet/mobile display.

This means the default bootstrap striping takes those into account, and "stripes" the table even - displayed row odd - hidden row even - displayed row odd - hidden row

So the stripe doesn't actually work.

This is why the logic is present in the blades. If you do not use collapsed columns, and do not intend to then absolutely the default bootstrap stripe behaviour will work.

shawe commented 6 days ago

I do some tests, and I think that is more easy to show you than explain it ;)

On my class that extends DataTableComponent:

Before

            ->setTableAttributes([
                'class' => 'table-striped table-hover table-bordered'
            ])

After

            ->setTableAttributes([
                'class' => 'table table-hover table-bordered'
            ])

Based on your recomendation to change it (for me this laravel-livewire-tables-odd will be in the original view):

        $this->setTrAttributes(function ($row, $index) {
            if ($index % 2 === 0) {
                return [
                    'default' => false,
                    'class' => 'rappasoft-striped-row',
                ];
            }
            return [
                'default' => false,
                'class' => 'laravel-livewire-tables-odd rappasoft-striped-row',
            ];
        });

I have a fixes.scss, that is imported in my app.scss that allows me to fix some minor problems like that:

// This was added some time before
.table-striped > tbody > tr:nth-of-type(odd) > * {
    --bs-table-bg-type: var(--bs-body-bg) !important;
}

.laravel-livewire-tables-odd {
    --bs-table-bg-type: var(--bs-body-bg) !important;
}

npm run build, and its ready.

Native BS5 table-striped, with nth-of-type(odd) fix:

The .laravel-livewire-tables-odd class result:

Maybe you need to provide a file like bootstrap-custom-fixes.scss file like this to globally allow this kind of integration?

I suggested from start that the issue was bg-light and bg-white for this reason. BS5 is applying this colors to a different way than you, for this reason previously doesn't look like stock BS5.

For me looks more better and clear with this proposal, and is not forcing to others to replace the default classes when the global work is well, only this minor improvement ;) I hope you like this option more.

shawe commented 6 days ago

Looking this screenshots that I provide you, I'm thinking that I'm watching inverted the -/+ and the next row that must be in close state by default.

When I see + row hasn't d-none When I see - row has d-none

rappasoft / laravel-livewire-tables