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

"Agnostic" filters - decouple filters from datatables #5714

Open
wants to merge 9 commits into
base: next
Choose a base branch
from

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Nov 6, 2024

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.

image

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.

@pxpm pxpm assigned tabacitu and pxpm Nov 6, 2024
@pxpm pxpm added the v7 label Nov 6, 2024
@susanu
Copy link
Contributor

susanu commented Nov 27, 2024

Off topic: Would you consider adding support for global filters?
Example: A filter that will be displayed on all pages, lets say a date range filter.
On the cruds that have a date range filter, just take the values from the global one (if you set it this way).
Use case: You want to see statistics about last week on multiple cruds and don't want to set the date range every time you navigate to the crud.

Cheers!

@tabacitu
Copy link
Member

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?

@susanu
Copy link
Contributor

susanu commented Nov 28, 2024

Hi @tabacitu,

I'm better at writing code than articles.
After v7 is released and filters can be reused in other places, i will submit a pull request, when time allows it, for this feature.

Cheers!

@pxpm
Copy link
Contributor Author

pxpm commented Dec 2, 2024

@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 admin/invoices?date=2024-01-01. If the date filter exist on the invoice crud, it will be automatically applied.

Thanks for the suggestion, eagerly waiting for the PR 👍

pxpm and others added 4 commits December 3, 2024 10:19
* add method to get ajax uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* use an abstract class

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* refactor uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* refactor rules

* Apply fixes from StyleCI

[ci skip] [skip ci]

* move ajax to PRO, cleanup

* Apply fixes from StyleCI

[ci skip] [skip ci]

* make attributes available for all subfields

* fix tests

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* upload multiple and upload properly working 🙏

* fixes

* Apply fixes from StyleCI

[ci skip] [skip ci]

* allow the configuration of valueWithoutPath call.

* fix valid upload inside repeatables

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix condition

* cleanup

* fix

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix use case for enabling validation after entry is created

* Apply fixes from StyleCI

[ci skip] [skip ci]

* dont save array keys

* fix ajax validation

* fix validation messages

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fixes ValidUpload

* Apply fixes from StyleCI

[ci skip] [skip ci]

* dont json encode if casted in the model

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix previous file identification in repeatable

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix getting values

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add fake fields support

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip add uploaders tests

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add pro columns

* fix test suite

* fix tests

* ffix tests

* remove unused test views

* add uploaders to test coverage

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add coverage folder to gitignore

* make tests run faster by not reloading db when not necessary

* add coverage to validation tests

* add fake tests to uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add more tests

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add more upload assets

* fixes

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix single file

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add image column

* fix tests

* Apply fixes from StyleCI

[ci skip] [skip ci]

* remove hardcoded macro names

* Apply fixes from StyleCI

[ci skip] [skip ci]

* remove double loop, fix single file uploader

* Apply fixes from StyleCI

[ci skip] [skip ci]

* use a big increments and unsigned for primary key

* handle pivot file deletion

* Apply fixes from StyleCI

[ci skip] [skip ci]

* register events for relation models

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix typo

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix relationship uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* update temporary time key

* save objects in the macro

---------

Co-authored-by: StyleCI Bot <[email protected]>
* wip

* clean up

* add get uploadedFilesFromRequest to clean up uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix type cast

* add summernote uploader

* update summernote

* add summernote

* add more test assets

---------

Co-authored-by: StyleCI Bot <[email protected]>
Base automatically changed from allow-datatables-url-configuration to next December 5, 2024 11:07
Copy link
Member

@tabacitu tabacitu left a 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.

src/resources/views/crud/inc/datatables_logic.blade.php Outdated Show resolved Hide resolved
src/resources/views/crud/inc/datatables_logic.blade.php Outdated Show resolved Hide resolved
@tabacitu tabacitu assigned pxpm and unassigned tabacitu Dec 5, 2024
pxpm and others added 3 commits December 6, 2024 11:44
@pxpm pxpm force-pushed the agnostic-filters branch from 6e40e3a to a719f19 Compare December 6, 2024 11:45
pxpm and others added 2 commits December 10, 2024 17:02
@pxpm pxpm changed the title [POC] "Agnostic" filters "Agnostic" filters - decouple filters from datatables Dec 13, 2024
@pxpm pxpm assigned tabacitu and unassigned pxpm Dec 13, 2024
Copy link
Member

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?

Copy link
Member

@tabacitu tabacitu Dec 20, 2024

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 🤷‍♂️

Copy link
Contributor Author

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants