ghiscoding / slickgrid-universal

Slickgrid-Universal is a monorepo which includes all Editors, Filters, Extensions, Services related to SlickGrid usage and is also Framework Agnostic
https://ghiscoding.github.io/slickgrid-universal/
MIT License
83 stars 26 forks source link

feat(filters): add a `filterPredicate` option for user customization #1528

Closed ghiscoding closed 4 months ago

ghiscoding commented 4 months ago

TODOS

stackblitz[bot] commented 4 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.8%. Comparing base (9171b96) to head (0933015). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1528 +/- ## ======================================== + Coverage 99.8% 99.8% +0.1% ======================================== Files 198 198 Lines 21616 21621 +5 Branches 7219 7221 +2 ======================================== + Hits 21555 21560 +5 Misses 55 55 Partials 6 6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ghiscoding commented 4 months ago

@zewa666 I wouldn't mind a PR review, it brings a filterPredicate to allow user to provide his own filter predicate which answers this SO Angular-Slickgrid Filter. You can ignore the patchDataViewFilter(), it was only meant to provide a way to answer the person's request and I will remove it before merging.

Currently the filterPredicate is located inside the filter (ColumnFilter interface) because I think it makes sense to put it in there. Side note though, we do have a sortComparer located inside the Column interface root, but I still think the filter predicate should be inside the filter. The only thing though is that I wasn't sure about the name because we have the word filter twice filter.filterPredicate but simply predicate might confuse people, so I think filterPredicate is less confusing even if it's inside filter, what do you think?

https://github.com/ghiscoding/slickgrid-universal/blob/4284d5834ec9955a327043565169b99ede8988f4/packages/common/src/interfaces/column.interface.ts#L339-L340

zewa666 commented 4 months ago

so the predicate would essentially by-pass the built-in filter service filtering function? what happens to the operators though?

I'm actually very interested in the exact same case searching using an asterisk character but I thought of that more along the lines of being a new operator and also working for the OData backend with either eq operators including the wildcard or a custom odata function.

ghiscoding commented 4 months ago

so the predicate would essentially by-pass the built-in filter service filtering function?

yes

what happens to the operators though?

hmm yeah I forgot about the operator and after testing it locally, the operator is parsed prior to the filter predicate call and is included in the searchFilterArgs, so in theory the user could also customize that if he wants as well (I mean he could define any other token to compare and analyze it in the predicate if he wants and skip the operator completely if he wishes). BTW, the searchFilterArgs was meant to be used only internally because I separated my code as much as possible for filter condition stuff and I needed some filter info for running the conditions, and I think it's a good thing to expose it in the new filterPredicate because it comes for free so...

filter: {
  filterPredicate: (dataContext, searchFilterArgs) => {
     console.log(searchFilterArgs);
  }
}

is giving this console, it is currently set as Contains because that is the default operator, but if the operator prop is changed it should show up as well. The operator is analyzed before the call to the filter predicate, the reason it's called before is because we want to analyze it only once and not doing this work for every data row which would kill perf

image

if I type "*Ta" then the operator becomes "EndsWith" which is the typical operator detection. If the user wants to use something else as operator he could just parse the search value for whatever token he wants to use.

image

I also added a OperatorType.Custom in previous PR #1526 which would probably be more useful for customizing backend services, by itself the OperatorType.Custom does nothing, I just added to make it a valid operator in case the user wants to use his own customized backend

zewa666 commented 4 months ago

oh ok thats great, didnt notice that one. guess thats also on the next branch right now? while at it can we also define what the visible text will be in the operator dropdown? e.g for the like I'd like to have "a*b" as that is neither starts nor ends with but also not contains.

back to the original question though, ideally the dev wouldnt pick a filter type that allows to select an operator if he provides a custom filterPredicate in order to reduce UI complexity for the users.

filterPredicate sounds good as well as predicate alone might not be intuitive enough as you said

hmm also note your implementation is not exactly correct as while %Ta% might be a contains what about %Ta%1? So the wildcard is treated like a regex dot-character zero or multiple times.

here is an example https://codereview.stackexchange.com/a/36864

ghiscoding commented 4 months ago

oh ok thats great, didnt notice that one. guess thats also on the next branch right now?

I merged it this morning so it won't be available until I publish a new release, v5.1 of universal

while at it can we also define what the visible text will be in the operator dropdown? e.g for the like I'd like to have "a*b" as that is neither starts nor ends with but also not contains.

You can already provide alternate texts as per docs (because someone requested it). However there's no way at the moment to change the list of compound operators, that is currently analyzed by the column type, I guess we could provide a way to override those too.

https://github.com/ghiscoding/slickgrid-universal/blob/4284d5834ec9955a327043565169b99ede8988f4/packages/common/src/filters/inputFilter.ts#L220-L241

https://github.com/ghiscoding/slickgrid-universal/blob/4284d5834ec9955a327043565169b99ede8988f4/packages/common/src/filters/filterUtilities.ts#L125-L152

Maybe what we should do is to modify the current applyOperatorAltTextWhenExists() (or use a new name) to not just override alternate texts but instead return a complete list of compound operators and I guess we could do that today without too much docs change since the doc wasn't clear enough about it being only for text

https://github.com/ghiscoding/slickgrid-universal/blob/4284d5834ec9955a327043565169b99ede8988f4/packages/common/src/filters/filterUtilities.ts#L160-L171

ghiscoding commented 4 months ago

back to the original question though, ideally the dev wouldnt pick a filter type that allows to select an operator if he provides a custom filterPredicate in order to reduce UI complexity for the users.

The user can provide a filter.operator at any time though, no need for compound operator in that case. I know we should probably have a way to provide more custom operator in the future but it's not quite possible at the moment because I have a fixed OperatorType (or OperatorString), and the OperatorType.custom is just quick hack to make it compile without errors.

hmm also note your implementation is not exactly correct as while %Ta% might be a contains what about %Ta%1? So the wildcard is treated like a regex dot-character zero or multiple times.

bahh I don't care that much, I did improve it but forgot to push the commit (which I just did). I mean, I spent like 2 hours trying to get it working with regex and just gave up and using .split() is a quick and dirty way to do it (I would prefer regex ideally). After updating the code, I added an animated gif in the SO which shows what the user was looking for (more or less).

I guess more filter features could be added in separate PRs.

zewa666 commented 4 months ago

as for this PR itself I think that's fine as is. everything else would be a different, although related topic, and hence worth another PR.

ghiscoding commented 4 months ago

hahah taking another look at my own docs, I forgot that I actually already done the change for a completely custom compound list.. dohhhh 😆

See Compound Operator List (custom list) which is just above the section "Compound Operator Alternate Texts".... isn't that great, when you just realized there's nothing more to do haha


The only thing left I guess would be to provide custom operators, I still don't know how we could do that though. Because, currently I use OperatorType (which is an enum) and/or OperatorString (which is a type) and after googling a little, I couldn't really find a way to extend both in TS

I don't see a way to address this without a breaking change, which I'm not looking to do at all. Not just because I just released a breaking change version but mostly because of usability, it's easy as it is today to write either type: OperatorType.greaterThan or type: '>=' (both are currently accepted). The only other way would be to have something like type: OperatorType | OperatorString | any (or unknown maybe) but that would kill the intellisense which is not ideal either, so... no idea on how to fix this

in theory, I could still do breaking changes if I want, because I would very surprised if anyone upgraded yet in Aurelia-Slickgrid/Slickgrid-React. BTW, are you still using Aurelia or..? It's taking forever to release Au2 but recently bigopon mentioned they're getting closer to RC (better late than never I guess)

zewa666 commented 4 months ago

not actively using Aurelia any longer sadly due to timeconstraints and lack of projects.

thx for all the info, I guess with the one new Operator.Custom I should be all set for my needs

ghiscoding commented 4 months ago

I guess with the one new Operator.Custom I should be all set for my needs

I don't mind adding couple of extra operators (like a*b that you mentioned above) if it makes sense. But if we do, it would have to be used somewhere (I'm assuming OData in your use case).