-
Notifications
You must be signed in to change notification settings - Fork 908
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
"Agnostic" filters - decouple filters from datatables #5714
base: next
Are you sure you want to change the base?
Conversation
Off topic: Would you consider adding support for global filters? Cheers! |
Hi @susanu - I think that's an interesting idea, but a very niche one. I see how it would be useful (I have a company select in one project too). But I don't think Backpack needs to help with it... it's ok for the dev to do it custom, since it's IS very custom, and particular to that project. We're not looking to add any "could-have" features, only "must-have" and "should-have". But it would make for a very interesting article - I'll give you that! Wanna write it? |
Hi @tabacitu, I'm better at writing code than articles. Cheers! |
@susanu "theoretically" that's already possible. You can store the "global" filter (you can't use the native filters (yet!)) in a session variable, and add a middleware, or similar approach, that reads from that session variable and redirects the user to say Thanks for the suggestion, eagerly waiting for the PR 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with making it a component. I agree with the naming. Please redo the event names so it's backpack:filters:clear
like I said in https://github.com/Laravel-Backpack/PRO/pull/296 too, merge next
into this, test it, and give it back.
6e40e3a
to
a719f19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we move all this code back to crud::inc.filters_navbar
... and have the component blade file just do @include('crud::inc.filters_navbar')
? That would give us both the benefit of having a component people can easily use... and make this less of a breaking change... right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or waaaait. Since FILTERS are a PRO feature... wouldn't it make more sense to move the component and blade file.... inside the PRO package? 👀 If we are to move files around... at least we would solve that issue too 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In part it makes sense, the filters we have are all in PRO, but from other perspective "filters" != "filters framework". WDYM ? Well, filters_navbar
it's the "suppport" for any filters, ours in PRO and any custom filters developers may have. So we ship the "filter framework" in the core, but we built our filters in PRO.
I imagine a scenario where we develop a package and that package has their own filters (not using the pro ones), if we move this to PRO, that package must depend on PRO just to have the filters navbar where it can add the filters. Also, the CrudFilter
class is in core not in PRO, so I think it makes sense to keep this on the core and not move it to pro.
Your call 👍
1253d3b
to
7019815
Compare
[ci skip] [skip ci]
[ci skip] [skip ci]
49d8f33
to
1d88043
Compare
WHY
BEFORE - What was wrong? What was happening before this PR?
Filters were tightly coupled with the datatable making it impossible to use in a page that didn't had the datatable.
AFTER - What is happening after this PR?
Filters can be used in any page, and developer can configure the actions to perform when they are enabled/disabled.
HOW
How did you achieve that, in technical terms?
transformed the filters_navbar into a component that can be reused.
refactored the filters to emit events when they are changed.
Is it a breaking change?
If you have
filters_navbar.blade.php
overwritten yes it is.