GroupItemMetaDataProvider implements SlickPlugin #955
Conversation
As it needs to be registered as plugin on grid to work properly, it should implement SlickGrid plugin interface, so typechecking of `grid.registerPlugin(groupItemMetadataProvider)` succeeds.
…ry value Grouping logic groups items based on equality of returned grouping values; probably does not need to be string. For example, we are using it to group based on Date value (by e.g. rounding them to nearest whole hour). If string is required by the grouping function, additional conversion of Date to string is needed, which we would rather avoid for perf reasons.
| @@ -3,7 +3,7 @@ import { GroupingComparerItem } from "./groupingComparerItem.interface"; | |||
| import { GroupingFormatterItem } from "./groupingFormatterItem.interface"; | |||
| import { SortDirectionNumber } from "./sortDirectionNumber.enum"; | |||
|
|
|||
| export type GroupingGetterFunction<T = any> = (value: T) => string; | |||
| export type GroupingGetterFunction<T = any> = (value: T) => any; | |||
There was a problem hiding this comment.
why are you converting this to any? It should always be a string since we're referencing a column name
There was a problem hiding this comment.
I will open two separate PRs (one for GroupItemMetaDataProvider and other for GroupingGetterFunction. I accidentally created MR from our master branch
About any; not sure if it is the right approach; GroupingGetterFunction is free to return anything I guess, only equality of the returned values is taken into account in the underlying slickgrid logic. I guess it is valid that grouping getter returns number for some rows string for others? Not sure how to type that differently than any? Maybe unknown would be better to force explicit typing when specific return type of grouper might be needed?
There was a problem hiding this comment.
I again disagree, it should always be a string, we're always looking to return a column name (name). This is not about what is being grouped, it's about the column name to find via the getter property of the group. Unless I missed something, if so prove me wrong because I won't accept this being changed from string
|
Will close this pull request and open two separate PR (should not be creating MR from mater branch of our fork) |
As it needs to be registered as plugin on grid to work properly, it should implement SlickGrid plugin interface, so typechecking of
grid.registerPlugin(groupItemMetadataProvider)succeeds.