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

new feature: searchable() #233

Merged
merged 14 commits into from
Apr 25, 2021

Conversation

bdelamatre
Copy link
Contributor

Follow-up to #210

  • re-added searchable() method to columns
  • added applySearchFilter() which applies default search behavior to searchable() columns
  • utilities class to replace pre 1.0 trait functionality
  • tests

@rappasoft
Copy link
Owner

Gonna need to have some actual documentation after all these features we're plowing into this thing.

{
return $this->rowsQuery();
// sorting?
if (method_exists($this, 'applySorting')) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these two methods always exist since they are part of the package and not user defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are defined in traits, so as long as the traits are applied to the class. I could go either way given this implementation if you have a preference.

In general I assume that traits are meant to be mixed and matched. So, maybe there is a future scenario where someone would need a table component but without specific traits.

E.g. I could see on #200 the possibility of creating a separate component for collections which may call for excluding these traits (or mixing in others).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, though in the case of this package even though the traits are functional, they were more meant for organizational purposes. Having them doesn't hurt though.

@rappasoft rappasoft added the Awaiting Next Release Currently merged into development awaiting a release to master label Apr 25, 2021
@rappasoft
Copy link
Owner

Ok I have some cleanup done locally and everything is working well, I actually can't find a need for the old way with my test table. The only thing I can't get to work is the searchable closure, no matter what I add in there the search just returns nothing altogether. Since you have a better idea of how it works can you just see if it's working correctly, and then maybe put a little blurb in the readme?

@rappasoft rappasoft merged commit ff10f30 into rappasoft:develop Apr 25, 2021
@rappasoft
Copy link
Owner

Since I merged this into develop, if you end up finding an issue or want to add to the readme I'll just merge a new PR if you don't mind. I'm going to try to dig into why I can't get it to work tomorrow.

This was a great PR by the way that's a lot of work and we all appreciate it.

@bdelamatre
Copy link
Contributor Author

Thanks!

I see the issue with callback and submitted a separate PR:
#234

However, I missed your note there about updating the readme.

@rappasoft rappasoft mentioned this pull request Apr 25, 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