filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
18.86k stars 2.93k forks source link

Sorting icon in table missing #3861

Closed lloricode closed 2 years ago

lloricode commented 2 years ago

Package

filament/tables

Package Version

v2.15.44

Laravel Version

v8.83.23

Livewire Version

v2.10.7

PHP Version

PHP 8.0.22

Problem description

sort icon on table did not appear, but sorting is still working, i seen this PR merge https://github.com/filamentphp/filament/pull/3801, and try to revert this on my vendor and its back to normal

i get this error on console tab

Uncaught ReferenceError: $isSortColumn is not defined
    at eval (eval at <anonymous> (module.esm.js:471:14), <anonymous>:3:32)
    at module.esm.js:489:21
    at V (module.esm.js:409:12)
    at module.esm.js:2623:17
    at n (module.esm.js:1609:16)
    at Object.effect (module.esm.js:1584:5)
    at r (module.esm.js:48:33)
    at module.esm.js:64:27
    at Function.<anonymous> (module.esm.js:2623:3)
    at n (module.esm.js:561:7)

Expected behavior

sort icon will still working

Steps to reproduce

make sure you sorting on table is working and has it icon, then upgrade to a latest table version, take a look on icon will dis-appear and check the browser console tab

Reproduction repository

https://github.com/lloricode/laravel-filament-issue/tree/table-sort-icon

Relevant log output

module.esm.js:420 Uncaught ReferenceError: $isSortColumn is not defined
    at eval (eval at <anonymous> (module.esm.js:471:14), <anonymous>:3:32)
    at module.esm.js:489:21
    at V (module.esm.js:409:12)
    at module.esm.js:2623:17
    at n (module.esm.js:1609:16)
    at Object.effect (module.esm.js:1584:5)
    at r (module.esm.js:48:33)
    at module.esm.js:64:27
    at Function.<anonymous> (module.esm.js:2623:3)
    at n (module.esm.js:561:7)
github-actions[bot] commented 2 years ago

Hey @lloricode! We're sorry to hear that you've hit this issue. 💛

However, it looks like you forgot to fill in the reproduction repository URL. Can you edit your original post and then we'll look at your issue?

We need a public Git repository which contains a Laravel app with the minimal amount of Filament code to reproduce the problem. That would allow us to download it and review your bug much easier, so it can be fixed quicker. Please make sure to include a database seeder with everything we need to set the app up quickly.

lloricode commented 2 years ago

Screenshot_2022-09-05_09-10-43

github-actions[bot] commented 2 years ago

Thank you for providing reproduction steps! Reopening the issue now.

zepfietje commented 2 years ago

It seems like the Blade <x-dynamic-component> is not being compiled at all. Might this be due to the Laravel version you use?

Really weird, as I bet you should have many more issues in your project if that component doesn't work correctly.

zepfietje commented 2 years ago

Laravel 8 supports dynamic Blade components, so I'm unsure what exactly goes wrong in your project. This isn't a Filament issue, but most probably a problem with your setup. So I'll close this issue.

danharrin commented 2 years ago

I've seen this sort of issue before and I cant remember the fix.

lloricode commented 2 years ago

Laravel 8 supports dynamic Blade components, so I'm unsure what exactly goes wrong in your project. This isn't a Filament issue, but most probably a problem with your setup. So I'll close this issue.

but the https://github.com/lloricode/laravel-filament-issue/tree/table-sort-icon is a fresh laravel 8 with just a basic filament user resource, so its a laravel issue?

danharrin commented 2 years ago

Yes, there is some reason why Blade components are not being compiled properly in your setup

jonnott commented 2 years ago

Yes, there is some reason why Blade components are not being compiled properly in your setup

I'm having the same issue, with the first resource, in a brand new (fresh Laravel app) Filament Admin setup.

Is it also a problem I specifically have with compilation of Blade components?

danharrin commented 2 years ago

@jonnott could you share the repo with us?

jonnott commented 2 years ago

@jonnott could you share the repo with us?

Not easily, it's just local at the moment, a fresh project.

If it's any help, this is the exact error I'm seeing in Chrome DevTools:

image

This happens even if I haven't yet set a defaultSort() on the $table.

I'm running: very latest filament 2.x, latest Laravel 8.x, on PHP 8.0.8.

danharrin commented 2 years ago

I have a suspicion that this is caused by the @class directive. It's a Laravel 8 bug that means the dynamic component is not compiled if it contains nested Blade directives.

I've just released v2.15.49. Does this fix the problem? Make sure to run artisan filament:upgrade after.

jonnott commented 2 years ago

Sorry, I've done the upgrade and the error is the same :(

Of course I have filament:upgrade in my post-update-cmd, like a good boy ;)

danharrin commented 2 years ago

That's a shame! Will definitely need a reproduction repo.

It's a fresh project, why not Laravel 9?

jonnott commented 2 years ago

It's a fresh project, why not Laravel 9?

It's complicated .. project was created when L8.x was current, added a model or two .. then stalled .. now resumed work on it, added Filament admin.

I guess a logical step would be to upgrade it to L9.x, but I'd need to carve out a little time to do that.

jonnott commented 2 years ago

I have a suspicion that this is caused by the @class directive. It's a Laravel 8 bug that means the dynamic component is not compiled if it contains nested Blade directives.

Has this bug ever been fixed in Laravel 8? Is it because Laravel 8 is only getting security fixes now?

danharrin commented 2 years ago

I don't even know if that was the bug or not, it was a guess. I removed the @class usage in the latest version and you said its not fixed, so it sounds like I was wrong.

zepfietje commented 2 years ago

Makes me wonder why specifically this dynamic component doesn't compile and it works fine in other places. 🤔

jonnott commented 2 years ago

I don't even know if that was the bug or not, it was a guess. I removed the @class usage in the latest version and you said its not fixed, so it sounds like I was wrong.

Well, now I've upgraded my project to Laravel 9, and all is dandy with the sorting!

My only wish with it now is .. if there was a way of doing the default ordering without it having to add query-string values to the URL, i.e. /admin/users?tableSortColumn=created_at&tableSortDirection=desc .. could this somehow be passed through another way, leaving the URL clean (/admin/users) ?

danharrin commented 2 years ago

Yes, on the ListUsers page you can add the $queryString = [] property which will prevent Livewire from adding anything to the query string

jonnott commented 2 years ago

Yes, on the ListUsers page you can add the $queryString = [] property which will prevent Livewire from adding anything to the query string

I don't think that's really what I'd want to achieve .. if any 'parameters' have been changed from the default view (i.e. just /admin/users) e.g. via changing to a different sort order, then I'd want that to be in the query-string, so that exact state of the page can be copy/pasted as a link. But in the case where the sort ordering is as per the configured default, then ideally those sorting parameters wouldn't need to be included on the URL. I guess I'm asking if there's other way that the default ordering can be passed to the front-end component, rather than also having to append it to the overall page's query-string (only in the case of the default sorting).

zepfietje commented 2 years ago

@lloricode, could you confirm if this is still an issue in the latest Filament version or it has been resolved?

lloricode commented 2 years ago

@lloricode, could you confirm if this is still an issue in the latest Filament version or it has been resolved?

yes, possible tomorrow will check this,

zepfietje commented 2 years ago

I've checked with your reproduction repository, @lloricode. Updating Filament to the latest version does fix the issue.

@danharrin, your fix did work then.

lloricode commented 2 years ago

I've checked with your reproduction repository, @lloricode. Updating Filament to the latest version does fix the issue.

@danharrin, your fix did work then.

will comfirm this

lloricode commented 2 years ago

I've checked with your reproduction repository, @lloricode. Updating Filament to the latest version does fix the issue.

@danharrin, your fix did work then.

confirmed already with with v2.16.2

thanks @danharrin :100: