igniteui-angular icon indicating copy to clipboard operation
igniteui-angular copied to clipboard

Rework default sorting/filtering strategy's and their methods to be grid agnostic.

Open MayaKirova opened this issue 3 years ago • 7 comments

Currently the sorting/filtering strategy methods sort/filter accept an optional parameter: grid. This should be removed and if needed another optional parameter can be exposed in case some data is needed from the grid.

For pivot grid a custom value extraction function (memberFunction) is needed to extract the values to sort/filter. TreeGrid also seems to use the grid reference for something and may need to be modified.

This came up from: https://github.com/IgniteUI/igniteui-angular/pull/10147#discussion_r787893908

MayaKirova avatar Jan 31 '22 13:01 MayaKirova

@ChronosSF @rkaraivanov @dkamburov I've noticed that he base filter/sort strategy also use the grid to get some information from the columns (like dataType or formatter): https://github.com/IgniteUI/igniteui-angular/blob/master/projects/igniteui-angular/src/lib/data-operations/filtering-strategy.ts#L65 https://github.com/IgniteUI/igniteui-angular/blob/master/projects/igniteui-angular/src/lib/data-operations/filtering-strategy.ts#L134

Does it sound reasonable if we remove the grid reference to store the datatype or the column ref as optional parameter in the Filtering/Sorting expression?

MayaKirova avatar Feb 02 '22 14:02 MayaKirova

So looking at filtering there seems to be 2 points in which grid is used.

  1. to get the column formatter in case the column should be filtered with a formatter

Here we can decide that the formatter is not a strategy option but rather a generic "value modifier" function that can be passed to the filter as a parameter by the grid. In fact this may allow a filter strategy with formatting on top to be used by the Combo for example. (not sure but right now Combo probably doesn't use the filter strategies at all).

  1. to see if the value is Date because it may be stored as a string.

I believe this can be dropped entirely. Date and Time expressions are entirely different from string afaik and can be used to determine if the rec values should be parsed or not. This will also simplify some functions that currently pass these as parameters for no particular reason.

Sorting certainly makes it more troublesome though. I am not sure if sorting a date column as string produces the correct result 100% of the time, probably not, though maybe... Additionally you want to know if you are case sensitive or not. The only way I see the grid can be omitted is to enhance the sorting expressions really. The grid can easily set expressions that add the metadata for the sort, such as if it is a date/time sort, if it should be case sensitive, etc. It might even be beneficial from an API standpoint to have more info-rich expressions.

ChronosSF avatar Feb 07 '22 13:02 ChronosSF

Btw, researching if you can sort the string values for Date-s with the same results could be worth doing as this parsing on each data op is certainly slow. There certainly are benefits to the full data binding/transformation capabilities of igGrid ;)

ChronosSF avatar Feb 07 '22 13:02 ChronosSF

Regarding 1.

The formatter is not one method for the whole grid. It's per column, so we will need to pass either a separate collection of value extractors or an additional prop for that extractor as part of the SortingExpression/FilteringExpression so that they can be associated by fieldName. Also if it's something that grid can optionally pass, the logic can no longer be encapsulated in a separate filter strategy, so we will need to deprecate/remove the FormattedValuesFilteringStrategy. Since it's public it will be a breaking change. Also, how will the grid decide whether it needs to pass formatters as custom extractors for filtering/sorting? Maybe a new prop?

Regarding 2.

Removing them for filtering and assuming by some arbitrary property of the filtering condition like the condition name or logic function, may also cause some breaking changes. If the user has defined some custom condition for a date/time column he would be getting the related date object right now, if we remove this as a whole and format as date only if it's one of the default date conditions this will no longer be the case.

MayaKirova avatar Feb 08 '22 16:02 MayaKirova

There has been no recent activity and this issue has been marked inactive.

github-actions[bot] avatar Apr 10 '22 00:04 github-actions[bot]

There has been no recent activity and this issue has been marked inactive.

github-actions[bot] avatar Jun 15 '22 00:06 github-actions[bot]

There has been no recent activity and this issue has been marked inactive.

github-actions[bot] avatar Aug 15 '22 00:08 github-actions[bot]

There has been no recent activity and this issue has been marked inactive.

github-actions[bot] avatar Nov 16 '22 00:11 github-actions[bot]