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

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

Merged
merged 3 commits into from
May 14, 2024

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented May 14, 2024

  • adding a filterPredicate would help user who want to have a custom filter predicate callback that cannot be handled by built-in filters
  • also added a patch filter version, that was only for testing purpose (will probably remove before merging) and to provide a way to answer this SO question: https://stackoverflow.com/questions/78471412/angular-slickgrid-filter

TODOS

  • unit tests
  • E2E tests
  • add documentation

- adding a `filterPredicate` would help user who want to have a custom filter predicate callback that cannot be handled by built-in filters
- also added a patch filter version, that was only for testing purpose (will probably remove before merging) and to provide a way to answer the SO question: https://stackoverflow.com/questions/78471412/angular-slickgrid-filter
Copy link

stackblitz bot commented May 14, 2024

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

Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 14, 2024

@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?

/** Custom Sort Comparer function that can be provided to the column */
sortComparer?: SortComparer;

@zewa666
Copy link
Contributor

zewa666 commented May 14, 2024

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
Copy link
Owner Author

ghiscoding commented May 14, 2024

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
Copy link
Contributor

zewa666 commented May 14, 2024

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
Copy link
Owner Author

ghiscoding commented May 14, 2024

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.

protected getCompoundOperatorOptionValues(): OperatorDetail[] {
const type = (this.columnDef.type && this.columnDef.type) ? this.columnDef.type : FieldType.string;
let optionValues = [];
if (this.columnFilter?.compoundOperatorList) {
return this.columnFilter.compoundOperatorList;
} else {
switch (type) {
case FieldType.string:
case FieldType.text:
case FieldType.readonly:
case FieldType.password:
optionValues = compoundOperatorString(this.gridOptions, this.translaterService);
break;
default:
optionValues = compoundOperatorNumeric(this.gridOptions, this.translaterService);
break;
}
}
return optionValues;
}

/** returns common list of string related operators and their associated translation descriptions */
export function compoundOperatorString(gridOptions: GridOption, translaterService?: TranslaterService): OperatorDetail[] {
const operatorList: OperatorDetail[] = [
{ operator: '', desc: getOutputText('CONTAINS', 'TEXT_CONTAINS', 'Contains', gridOptions, translaterService) },
{ operator: '<>', desc: getOutputText('NOT_CONTAINS', 'TEXT_NOT_CONTAINS', 'Not Contains', gridOptions, translaterService) },
{ operator: '=', desc: getOutputText('EQUALS', 'TEXT_EQUALS', 'Equals', gridOptions, translaterService) },
{ operator: '!=', desc: getOutputText('NOT_EQUAL_TO', 'TEXT_NOT_EQUAL_TO', 'Not equal to', gridOptions, translaterService) },
{ operator: 'a*', desc: getOutputText('STARTS_WITH', 'TEXT_STARTS_WITH', 'Starts with', gridOptions, translaterService) },
{ operator: '*z', desc: getOutputText('ENDS_WITH', 'TEXT_ENDS_WITH', 'Ends with', gridOptions, translaterService) },
];
// add alternate texts when provided
applyOperatorAltTextWhenExists(gridOptions, operatorList, 'text');
return operatorList;
}
/** returns common list of numeric related operators and their associated translation descriptions */
export function compoundOperatorNumeric(gridOptions: GridOption, translaterService?: TranslaterService): OperatorDetail[] {
const operatorList: OperatorDetail[] = [
{ operator: '', desc: '' },
{ operator: '=', desc: getOutputText('EQUAL_TO', 'TEXT_EQUAL_TO', 'Equal to', gridOptions, translaterService) },
{ operator: '<', desc: getOutputText('LESS_THAN', 'TEXT_LESS_THAN', 'Less than', gridOptions, translaterService) },
{ operator: '<=', desc: getOutputText('LESS_THAN_OR_EQUAL_TO', 'TEXT_LESS_THAN_OR_EQUAL_TO', 'Less than or equal to', gridOptions, translaterService) },
{ operator: '>', desc: getOutputText('GREATER_THAN', 'TEXT_GREATER_THAN', 'Greater than', gridOptions, translaterService) },
{ operator: '>=', desc: getOutputText('GREATER_THAN_OR_EQUAL_TO', 'TEXT_GREATER_THAN_OR_EQUAL_TO', 'Greater than or equal to', gridOptions, translaterService) },
{ operator: '<>', desc: getOutputText('NOT_EQUAL_TO', 'TEXT_NOT_EQUAL_TO', 'Not equal to', gridOptions, translaterService) }
];

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

// internal function to apply Operator detail alternate texts when they exists
function applyOperatorAltTextWhenExists(gridOptions: GridOption, operatorDetailList: OperatorDetail[], filterType: 'text' | 'numeric') {
if (gridOptions.compoundOperatorAltTexts) {
for (const opDetail of operatorDetailList) {
if (gridOptions.compoundOperatorAltTexts.hasOwnProperty(filterType)) {
const altTexts = gridOptions.compoundOperatorAltTexts[filterType]![opDetail.operator];
opDetail['operatorAlt'] = altTexts?.operatorAlt || '';
opDetail['descAlt'] = altTexts?.descAlt || '';
}
}
}
}

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 14, 2024

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
Copy link
Contributor

zewa666 commented May 14, 2024

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
Copy link
Owner Author

ghiscoding commented May 14, 2024

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)

@ghiscoding ghiscoding merged commit cbf64d8 into master May 14, 2024
9 checks passed
@ghiscoding ghiscoding deleted the feat/filter-predicate branch May 14, 2024 19:25
@zewa666
Copy link
Contributor

zewa666 commented May 14, 2024

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
Copy link
Owner Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants