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

add debounce to filters #5675

Merged
merged 4 commits into from
Oct 4, 2024
Merged

add debounce to filters #5675

merged 4 commits into from
Oct 4, 2024

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Sep 20, 2024

WHY

fixes: Laravel-Backpack/community-forum#131

BEFORE - What was wrong? What was happening before this PR?

It was annoying that anytime you click on a simple/checkbox filter, or raised/lowered the number in the range filter, a request was made to the server. That turned the experience a little bit cumbersome to use, plus very resource intensive.

AFTER - What is happening after this PR?

filters use now a "debounce function", so less calls are made to the search endpoint. there is a companion PRO PR: https://github.com/Laravel-Backpack/PRO/pull/280

HOW

How did you achieve that, in technical terms?

Used a debounce function, that prevents the same function to be called more than once in the given time period. That is now configurable setting debounce on the filter.

Is it a breaking change?

I think no.

@tabacitu
Copy link
Member

tabacitu commented Oct 4, 2024

I said on Discord on Sep 20th:

Sure, seems like a reasonable change. My question: Should we consider it breaking? If people have overriden filters_navbar.blade.php to change styles etc... they might not get the new functions, so calling them in PRO will trigger an error, right?

Then Pedro said on Discord on Oct 3rd:

Cristian, I am back on this topic.
Can you point me out what new functions you were refeering in PRO ? AFAIK we call the same function updateDatatablesOnFilterChange but with one additional parameter.

Even the new function, is only called inside the filters_navbar, so if you don't get the updated version, you will also not call the new function.
Can you give it another look and let me know your concerns ?

@tabacitu
Copy link
Member

tabacitu commented Oct 4, 2024

Here's the worst scenario I imagine:

  • someone wanted to change how filters look;
  • they updated their filter_navbar.blade.php and change some classes, styles etc;
  • they created a custom filter;
  • we push this update, which includes some functions in filters_navbar.blade.php and changes in all filter files to add a last parameter when calling updateDatatablesOnFilterChange()
  • they do a composer update;

What happens for them?

  • they get the changes in SOME filters;
  • they do NOT get the changes in filters_navbar.blade.php;
  • their custom filters will call updateDatatablesOnFilterChange() with one fewer parameter;

Since the last parameter of that function (debounce) has a default... I don't see any problem with merging this.

Agree with my logic?

@pxpm
Copy link
Contributor Author

pxpm commented Oct 4, 2024

Given your logic, if they had overriden the filters_navbar.blade.php (so they don't get the update), but they update PRO, their filters will call the updateDatatablesOnFilterChange() with one additional parameter. It will be ignore by the function if their function accept less parameters.

In case they update CRUD (and get the updated filters_navbar, but they don't update PRO, in that case it will happen what you said and the function will be called with one less parameter. But since the missing parameter has a default, that value will be used.

You can check this fiddle: https://jsfiddle.net/ozupgwt9/

Let me know if I am missing something, or is it still ok for you. 👍

@pxpm pxpm merged commit c2e22e4 into main Oct 4, 2024
8 checks passed
@pxpm pxpm deleted the add-debounce-to-filters branch October 4, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature Request] Debounce filters and search
2 participants