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

[Bug]: Problems with clean published views #1923

Open
shawe opened this issue Sep 3, 2024 · 9 comments
Open

[Bug]: Problems with clean published views #1923

shawe opened this issue Sep 3, 2024 · 9 comments
Labels
Conversation A topic discussion invalid This doesn't seem right wontfix This will not be worked on

Comments

@shawe
Copy link

shawe commented Sep 3, 2024

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

@shawe shawe added the bug Something isn't working label Sep 3, 2024
@lrljoe
Copy link
Collaborator

lrljoe commented Sep 9, 2024

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

lrljoe commented Sep 9, 2024

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

lrljoe commented Sep 10, 2024

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];
  });
}

@lrljoe lrljoe added invalid This doesn't seem right Conversation A topic discussion and removed bug Something isn't working labels Sep 10, 2024
@lrljoe lrljoe closed this as completed Sep 10, 2024
@shawe
Copy link
Author

shawe commented Sep 10, 2024

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

image

image

Without bg-light on tr:

image

image

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
Copy link
Author

shawe commented Sep 10, 2024

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 lrljoe reopened this Sep 12, 2024
@lrljoe
Copy link
Collaborator

lrljoe commented Sep 12, 2024

@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
Copy link
Author

shawe commented Sep 12, 2024

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:

image

image

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

image

image

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
Copy link
Author

shawe commented Sep 12, 2024

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

Copy link

stale bot commented Nov 4, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Conversation A topic discussion invalid This doesn't seem right wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants