Show all grid rows on grouping if paging is enabled

[Eralmidia]

Is your feature request related to a problem? Please describe.

If a grid has both paging and grouping, groups are not shown if they only exist on other pages than the current page.

Describe the solution you'd like

While grouping is on, it would be nice to have the option to ignore paging, in order to make sure that all group headers are shown.

Describe alternatives you've considered

I tried a solution where I simply increase the "perPage" from let say 100 to 1000 (max rows returned from the API) property in the onGroupingDone event. This works fine when applying grouping, but when the grouping is removed, it will render all 1000 rows (since the event is onGroupingDONE). Would it be possible to expose a onGroupingStarting event or similar? The old igniteui grid did have a lot more events in this regard, iirc? Note that we can't use virtualization for this grid, as we want to avoid double scrollbars on the page.

Additional context

Add any other context or screenshots about the feature request here.

[ChronosSF]

I think onGroupingStarting is painless enough to include and is probably the way to go for this use-case.

Any objections/comments @kdinev , @mpavlinov , @rkaraivanov

[kdinev]

@Eralmidia @ChronosSF Could it just be called onGrouping?

[rkaraivanov]

onGrouping sounds fine to me

[Eralmidia]

@kdinev @ChronosSF @rkaraivanov Hi Guys, a follow up on this one. We have looked more closely into this, and was wondering if it was possible to make the paging + grouping work more like on the igniteui grid? If we compare the two, there is a key difference, which makes the igniteui a lot better: While collapsed, in the old grid, each group only equates to a single row, as can be seen here: Then, when you expand a group: it will correctly apply paging as needed:

In the igx grid, all rows within the group is still counted towards the row per page when collapsed, meaning that we now have to move to other pages to find other groups.

So in reality, the grouping in the igx-grid is really only a "grouping per page", which is a lot less useful, and more confusing, since groups are hidden. I am also unsure whether the alternative solution I mentioned above really will work. Because say the grid has a default of 100 rows per page, with a max results of 1000. Then lets take the group on the Status column from the images above. The passed group (green checkmark) in this case has 774 rows, which still is too much to render. When expanding, those should have been spread out over 8 pages.

[ChronosSF]

If you are refering to the 'allRecord's paging mode for Ignite UI's GroupBy, it doesn't show all groups either. It's just that the group records are included in the paging cutoff but given enough groups even if they are all collapsed (> pageSize) they'll still span across multiple pages.

If this is the bahavior you need, we can discuss including it through an option. Originally we decided that the current one is better from an UX perspective. @kdinev , @rkaraivanov

[Eralmidia]

@ChronosSF Yes, I was specifically referring to the scenario in the pictures above. where you have a page of 100, and show all groups collapsed, they will all be on the first page (unless there was more than 100 groups of course), while on the igx-grid, groups gets hidden on later pages. My product manager actually refers to the grouping as "broken" currently 😛 In any case, adding it as an option would be great 😄

[kdinev]

@Eralmidia Something seems quite wrong with the current paging visualization. I think we should treat this as a bug, regardless of what is written in the spec. I just went over it and the way the paging integration is explained is not quite understandable.

[Eralmidia]

@kdinev I'm glad we can agree it doesn't quite work as intended 😉 Will this get fixed for 7.3.x or 8.x only?

[kdinev]

@Eralmidia We will target fixing it in both. We had a small discussion around how this can be handled to create the best user experience. We will add our change proposal to the specification and we will share it with you before implementation. What I don't want to have is the igGrid behavior where group headers are accounted for as real records in the paging, resulting in your record count being the records + all grouping headers, instead of just the records. This is what we were trying to avoid initially, but the result you pointed out is a flaw that we didn't account for.

[ChronosSF]

@Eralmidia , it actually works just like the dataRecordsOnly mode in igGrid. We chose this behavior because regardless of what expand/collapse state you have on a page, the paging state doesn't change when the end user collapses and expands. Depending on your use-case (average amount of records per group, page size, default expand state, etc) the behavior may look perfectly normal.

We'll change it in the spec / implementation to match the allRecords mode, however, that implementation itself has UX drawbacks. The major one is that big, expanded groups, the records for which span multiple pages will not have their group headers except on the page the group starts on. This is so the page size is consistent. Let me know if this works for you.

There is an alternative in which we do render group headers for each record group that doesn't have its group rendered naturally on the page, but then the pageSize is inconsistent and controlling such groups (expand/collapsing them) will cause pageIndex jumps that will further complicate the implementation and may be confusing from an end-user perspective.

[Eralmidia]

@kdinev Yes, I agree that the headers being counted as rows is a bit odd. Being able to avoid that but with the rest of it working as the old grid seems good.

@ChronosSF At least for us, missing headers on top of the page isn't really major drawback. Having it just at the start of the group is good enough, and and most of the time it's easy to see what group you are currently on, simply by looking at the column. I do realize that this might not be quite as easy if you use y-virtualization of course.

[ChronosSF]

@kdinev @Eralmidia , here is how the part of the spec concerned with the pipe order will look if we decide for the change:

Grouping is achieved through a pipe that must be called after the sorting one. It has the following structure:

export class IgxGridGroupingPipe implements PipeTransform {
    public transform(collection: any[], expression: ISortingExpression | ISortingExpression[],
                     expansion: IGroupByExpandState | IGroupByExpandState[], defaultExpanded: boolean,
                     id: string, pipeTrigger: number): any[] {
    }
}

It serves the purpose of processing the sorted data and creating the tree grouping structure based on the values equality. The values are compared with the SortingStrategy's comparer which can be overridden by the user. Afterwards, the tree is flattened based on the groups' expansion state producing a view that can be rendered with a structural directive (such as igxForOf).

This result of the grouping pipe is then sent to the paging one so that all group records participate in the paging process and are part of the total page size for each page. This can be observed in the following sample with a page size of 5:

Groups that span multiple pages are split between them. The group summary information is consistent for the whole group but the header itself is not created for each page:

Expanding and collapsing groups would change the paging state as it alters the total amount of items participating in paging:

[Eralmidia]

@ChronosSF Looks very good to me 😃

[kdinev]

@ChronosSF LGTM. Go ahead and PR it.

[Eralmidia]

@Eralmidia We will target fixing it in both. We had a small discussion around how this can be handled to create the best user experience. We will add our change proposal to the specification and we will share it with you before implementation. What I don't want to have is the igGrid behavior where group headers are accounted for as real records in the paging, resulting in your record count being the records + all grouping headers, instead of just the records. This is what we were trying to avoid initially, but the result you pointed out is a flaw that we didn't account for.

@kdinev Will this fix still also be for 7.3.x?

[kdinev]

@Eralmidia It's a behavioral change, so we introduced it in the 8.1.0 major. Do you have no plans to move to Angular 8?

[Eralmidia]

@kdinev Yes we do, but it's too late for the version we are currently about to release. But it's no big deal, we can list this as a known issue for this release, and move to 8 in the next one.