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

allow datatable url configuration #5713

Open
wants to merge 1 commit 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?

Datatables url was hardcoded to CRUD::getRoute(), by that reason it was not possible to use datatables in any other place than the main crud route, eg: $this->crud->setRoute('monsters'), would only allow a datatable in that specific url, trying to add a datatable in monsters/some-page would break things like filters, and require "hacking solutions" to try to overcome the search functionality.

AFTER - What is happening after this PR?

The datatable can be setup in any additional page by configuring the datatablesUrl, for that reason it's now possible to have filters and search without convoluted solutions.

HOW

How did you achieve that, in technical terms?

Replaced all $crud->getRoute() calls in datatables and filter scripts by $crud->getOperationSetting('datatablesUrl') and setup a default datatablesUrl ($crud->getRoute())` to keep it backward compatible.

Is it a breaking change?

No

@pxpm pxpm assigned tabacitu and pxpm Nov 6, 2024
@pxpm pxpm added the v7 label Nov 6, 2024
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 love it! Haven't tested, but the code changes are super-clear, and the impact is minimal. Love these simple PRs that add flexibility, without a compromise.

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

Successfully merging this pull request may close these issues.

2 participants