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

Search model options #211

Merged

Conversation

bdelamatre
Copy link
Contributor

Adds properties to compute model options on search property. This is presently set to .debounce.250ms in themes. This allows user to control those options without overriding the themes.

bdelamatre and others added 2 commits April 18, 2021 15:11
…tly set to .debounce.250ms in themes, this allows use to control those options without override the themes.
@rappasoft
Copy link
Owner

Should we be able to include the default of debounce 250ms instead of removing it?

@bdelamatre
Copy link
Contributor Author

I don't have an opinion on what the package default should be. If undefined, the livewire default is 150ms.

My own testing showed that:

  • 250ms wasn't enough of a debounce to prevent misfires while typing
  • On slower queries, misfires produced undesirable results.
  • Lazy loading was the most reliable method for slower queries as it eliminated misfires. However, it's not entirely clear from a UI perspective that you need move off the input to fire a search.

@rappasoft
Copy link
Owner

What if we just added 1 property of ?string $searchFilterProperty = null or something that the users would just add .lazy or .debounce.250ms or whatever they wanted? It would basically just append to the wire:model, if Livewire added different properties we wouldn't have to add anything. Am I missing a side effect of doing it this way?

@bdelamatre
Copy link
Contributor Author

That would work. Pros would be as you mentioned and it eliminates possible ambiguity in how options are preferred. Cons are that the options may be less evident and an increased risk that an incorrect value could break livewire.

I would ltrim the dot and manually add it to reduce that risk.

Do you want me to submit a different pull?

@rappasoft
Copy link
Owner

That's true both ways, I want to keep the codebase as small as possible for maintainability. They are both good options, the only confusion I would see with the 3 properties is someone accidentally using more than 1, and not understand which order they are being processed. Either way, we just need to document it well.

I would say the more Laravel way would be separating them into multiple properties for readability. So I think it's good, I'll pull and update the readme tonight when I get home.

@rappasoft rappasoft added the Awaiting Next Release Currently merged into development awaiting a release to master label Apr 19, 2021
@rappasoft rappasoft mentioned this pull request Apr 19, 2021
@rappasoft rappasoft merged commit 42ef090 into rappasoft:master Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Next Release Currently merged into development awaiting a release to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants