adds object/function filter to local plugin by benoj · Pull Request #747 · GriddleGriddle/Griddle

benoj · 2017-10-05T21:39:29Z

Griddle major version

1.9.0

Changes proposed in this pull request

Extends the functionality of the LocalPlugin in order to allow more customised filtering (closes #746). With this PR you can now call setFilter() with an object { [key1]: [filter], [key2]:[filter] } or function(row){ } or an object with functions {[key]: function() {} }this allows for filtering across multiple fields with different fields.

This allows for more customised behaviour. The object filter allows individual parts of the row to be filtered which can be used to build more complex filter UI. The default behaviour is AND across the object filter however you can also set the filter to be a function which is called with the entire row. This allows the user to do more logic in the filter e.g. XOR, OR etc.. on different row properties.

This is backwards compatible so any existing custom filters will work.
e.g.

Data:
{ id: 1, name: "Bob", age: 20} , { id: 2, name: "Alice", age: 49} , { id: 3, name: "Jimmy", age: 47}, { id: 4, name: "Bob", age: 21}

setFilter("Bob") // returns { id: 1, name: "Bob", age: 20}, { id: 4, name: "Bob", age: 21}

setFilter({ name: "bob") // returns { id: 1, name: "Bob", age: 20}, { id: 4, name: "Bob", age: 21}

setFilter({ name: "bob", age: function(age) { return age < 21; } ) // returns { id: 1, name: "Bob", age: 20}

setFilter({ name: "bob", age: function(age) { return age < 49; } ) // returns { id: 1, name: "Bob", age: 20},{ id: 3, name: "Jimmy", age: 47}, { id: 4, name: "Bob", age: 21}

setFilter(function(row) { return row.get("name") === "Bob" && row.get("age") < 21 } ) // returns { id: 1, name: "Bob", age: 20}

Why these changes are made

Needed this functionality for a project I am working on.

Are there tests?

Yes.

benoj · 2017-10-05T21:40:01Z

@dahlbyk ^ here is the PR as promised.

dahlbyk

This looks great! I do have one refactoring suggestion for performance's sake, but other than that...

Also, can you add a test to prove that filtering by multiple keys is an AND rather than OR?

It would also be neat to have a Storybook entry that shows how to use this new filter functionality, but we can fill that in later if you're not up to it now.

Thanks!

dahlbyk · 2017-10-07T18:42:49Z

-            value.toString().toLowerCase().indexOf(filterToLower) > -1;
-        }));
+    return data.filter(row => {
+      switch (typeof (filter)) {


For performance, can you flip this to process filter outside the data.filter(...)? This would work particularly well if you curry the functions above, e.g.

switch (typeof filter) { case 'string': return data.filter(textFilterRowSearch(columnProperties, filter)); case 'object': return data.filter(objectFilterRowSearch(columnProperties, filter)); case 'function': return data.filter(filter); default: return data; }

Will get onto it!

benoj · 2017-10-11T14:26:26Z

Just as an update – looking to make this PR (with changes) some point over the weekend as OSS and my work are hard to balance at the minute. Got it working locally on my machine :) for now anyway

dahlbyk · 2017-10-11T20:52:27Z

No worries, @benoj, we appreciate the contribution whenever you can finish it!

arnimhanke · 2018-04-10T11:39:59Z

Hi there, any updates for this feature? I realy need this functionality.

benoj · 2018-04-10T12:04:11Z

Hi @arnimhanke,

Sorry I have kinda dropped the ball on getting this PR complete as had so much on with work.

There are a few options for you

Wait for me to finish it (cant give a timeframe given we are trying to get a startup live in the coming few weeks)
https://www.npmjs.com/package/griddle-react-with-advanced-search use this - this is my forked version with the feature working - I dont recommend this long term.
Continue the work done and apply @dahlbyk suggestions

Cheers,

Ben

arnimhanke · 2018-04-11T06:12:53Z

Hi @benoj,

thx for the fast answer!
I can try to continue the work.

As I can see, there are two todos:
1. Performance optimization (Switch-Case)
2. New Test to prove that filtering by multiple keys is an AND rather than OR
Did I miss something?

Do I have to make a new PR or can i somehow use yours (I am new to Github)?

Greetings from Germany!

Arnim

benoj · 2018-04-11T06:17:28Z

Great news. I will provide you push access when I get into work. Just on train at the minute.

…

On Wed, 11 Apr 2018, 07:12 arnimhanke, ***@***.***> wrote: Hi @benoj <https://github.com/benoj>, thx for the fast answer! I can try to continue the work. As I can see, there are two todos: 1. Performance optimization (Switch-Case) 2. New Test to prove that filtering by multiple keys is an AND rather than OR Did I miss something? Do I have to make a new PR or can i somehow use yours (I am new to Github)? Greetings from Germany! Arnim — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#747 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACi6PJGAZ9stjwY-d1XtgTWAX_FsTeviks5tnZ7ogaJpZM4PvvMy> .

dahlbyk · 2018-04-11T13:45:11Z

+      if (filterable === false) {
+        return false;
+      }
+      return substringSearch(row.get(key), filter)


I just noticed we're missing semicolons on most of the new code.

Ah yeah - I tend to write mine without semi-colons. Might be worth adding an eslint or similar to ensure consistent styling

dahlbyk · 2018-04-11T13:55:13Z

As I can see, there are two todos:

Performance optimization (Switch-Case)

New Test to prove that filtering by multiple keys is an AND rather than OR

Did I miss something?

The last nice-to-have would be a story to demonstrate advanced filter usage.

Do I have to make a new PR or can i somehow use yours (I am new to Github)?

You can either make a new PR from a branch on your own fork, or you can push to @benoj's master branch (once he gives you access) and it will update this PR.

benoj · 2018-04-11T14:00:39Z

@arnimhanke I have added you as a collaborator on my branch so you should be able to push

waclock · 2018-04-11T18:13:19Z

Also looking forward to this feature, let me know if I can help.

arnimhanke · 2018-04-12T08:59:22Z

I am nearly done with the work.
But as far as I can see, the filtering is with multiple keys not AND, it is OR.
I wrote a test for that:
data to be filtered:
{ id: '1', name: 'luke skywalker' },
{ id: '3', name: 'luke skywalker' },
{ id: '2', name: 'han solo' },

filter: { id: '1', name: 'luke' }

result:
{ id: '1', name: 'luke skywalker' },
{ id: '3', name: 'luke skywalker' }

Is this the correct beahvior?

Greetings!

dahlbyk · 2018-04-12T16:27:54Z

But as far as I can see, the filtering is with multiple keys not AND, it is OR.

Is this the correct behavior?

I think AND is a more useful default.

This is why we write tests. 😀

arnimhanke · 2018-04-14T18:20:13Z

I think I am done with the changes, they are pushed to the master branch.
Did I miss something?
@dahlbyk And is implemented and there is a test for this behavior ;-)

dahlbyk

Thanks, @arnimhanke! I've added a few more tests for quirks I found, and refactored the object filter a bit, if you could take a look. I also rebased onto master to resolve the conflicts from #800 (cc @mbland). I seem to have screwed something up on that rebase, though, as my build just broke. 😞

I think we're getting close, though we still don't have any Storybook entries to demonstrate usage and prove that our tests are testing what we think they're testing. This sort of logic is easy to mess up, so I'd appreciate any additional eyes here.

arnimhanke · 2018-04-15T09:49:35Z

Hi,
the error is fixed (no failed tests) and i added a new story for the object-filtering.
From my side looks everything fine 😄
Is there anything else I can do?

benoj · 2018-04-15T16:09:32Z

should this be closed or merged?

dahlbyk · 2018-04-15T16:11:47Z

Git's acting weird, somehow I pushed upstream master to your master. 😞 Opening a new PR with the changes, sorry.

dahlbyk · 2018-04-15T16:15:11Z

Apparently I did git push https://github.com/benoj/Griddle.git master instead of git push -u https://github.com/benoj/Griddle.git HEAD:master. 😧

benoj · 2018-04-15T16:28:13Z

hmm, that is interesting given that you aren't a collaborator on the project. I would have expected you to get your push rejected...

dahlbyk · 2018-04-15T19:48:36Z

I have push bit on this repo, so I was able to push to your PR branch. 🙃 I was not able to push to your master after this closed.

KalalauEnterprises · 2018-04-30T18:47:49Z

Would love to use this now - I have a big need for this. What is the best way for me to get this? I'm currently in Meteor 1.6.1.1 with griddle-react npm package. Thanks!

arnimhanke · 2018-04-30T18:54:42Z

Just upgrade the version to 1.13.0
See: #807
You can find an example in the storybook
https://github.com/storybooks/storybook/blob/master/README.md

KalalauEnterprises · 2018-04-30T21:21:13Z

@arnimhanke Thank You!

KalalauEnterprises · 2018-04-30T22:46:27Z

The only examples I see in the stories are the filter by text and object. Is there an example of using setFilter with a function? I only ask because I need to filter for a date column >= myFilterDate along with a text match. Thanks!!

arnimhanke · 2018-04-30T23:01:01Z

There is no example for that.
Here is a short snippet from
"https://github.com/GriddleGriddle/Griddle/blob/master/src/plugins/local/selectors/__tests__/localSelectorsTest.js" (line 271):
function (row, i, data) {
return row.get("name") === 'luke skywalker';
}
I can give you a more detailed example tomorrow, if you want.

KalalauEnterprises · 2018-05-01T15:28:59Z

@arnimhanke No, that example is perfect. Thank you!

dahlbyk requested changes Oct 7, 2017

View reviewed changes

dahlbyk mentioned this pull request Feb 4, 2018

Column filters #86

Closed

dahlbyk reviewed Apr 11, 2018

View reviewed changes

dahlbyk force-pushed the master branch from 7825057 to 0907f9f Compare April 14, 2018 20:34

dahlbyk approved these changes Apr 14, 2018

View reviewed changes

dahlbyk force-pushed the master branch from 0907f9f to ca8b66c Compare April 14, 2018 20:52

dahlbyk requested changes Apr 14, 2018

View reviewed changes

dahlbyk closed this Apr 15, 2018

dahlbyk force-pushed the master branch from 90f9a13 to 8facf9a Compare April 15, 2018 16:08

dahlbyk mentioned this pull request Apr 15, 2018

adds object/function filter to local plugin #807

Merged

Uh oh!

Conversation

benoj commented Oct 5, 2017 • edited by dahlbyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Griddle major version

Changes proposed in this pull request

Why these changes are made

Are there tests?

Uh oh!

benoj commented Oct 5, 2017

Uh oh!

dahlbyk left a comment

Choose a reason for hiding this comment

Uh oh!

dahlbyk Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

benoj Oct 8, 2017

Choose a reason for hiding this comment

Uh oh!

benoj commented Oct 11, 2017

Uh oh!

dahlbyk commented Oct 11, 2017

Uh oh!

arnimhanke commented Apr 10, 2018

Uh oh!

benoj commented Apr 10, 2018

Uh oh!

arnimhanke commented Apr 11, 2018

Uh oh!

benoj commented Apr 11, 2018 via email

Uh oh!

dahlbyk Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

benoj Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

dahlbyk commented Apr 11, 2018

Uh oh!

benoj commented Apr 11, 2018

Uh oh!

waclock commented Apr 11, 2018

Uh oh!

arnimhanke commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dahlbyk commented Apr 12, 2018

Uh oh!

arnimhanke commented Apr 14, 2018

Uh oh!

dahlbyk left a comment

Choose a reason for hiding this comment

Uh oh!

arnimhanke commented Apr 15, 2018

Uh oh!

benoj commented Apr 15, 2018

Uh oh!

dahlbyk commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dahlbyk commented Apr 15, 2018

Uh oh!

benoj commented Apr 15, 2018

Uh oh!

dahlbyk commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KalalauEnterprises commented Apr 30, 2018

Uh oh!

arnimhanke commented Apr 30, 2018

Uh oh!

KalalauEnterprises commented Apr 30, 2018

Uh oh!

KalalauEnterprises commented Apr 30, 2018

Uh oh!

arnimhanke commented Apr 30, 2018

Uh oh!

KalalauEnterprises commented May 1, 2018

Uh oh!

benoj commented Oct 5, 2017 •

edited by dahlbyk

Loading

arnimhanke commented Apr 12, 2018 •

edited

Loading

dahlbyk commented Apr 15, 2018 •

edited

Loading

dahlbyk commented Apr 15, 2018 •

edited

Loading