Skip to content

Conversation

@idpaterson
Copy link

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
#1064 Sorting has no effect when used with row grouping

Summary:

  • sorts and column header sorting do not change the order of grouped rows in any way
  • Once grouped, table rows cannot be ungrouped

What is the new behavior?
Grouping is applied to the sorted internalRows rather than the raw rows input so any changes to sorting can affect the order of groups and of rows within groups.

Setting groupRowsBy to null will remove all row grouping. Care has been taken to allow the ngx-datatable-group-header markup shown in examples to not throw errors when rows are not grouped.

Rows are sorted lazily via accessors, so rapid changes to sorts and groupRowsBy may not incur as much overhead since they now just invalidate the sorted rows. Once sorted and grouped the rows are cached for subsequent access until invalidated.

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Tables with grouped rows for which sorting should not be allowed will need to set externalSorting to true as rows will now automatically sort in response to user interaction with the column headers. Perhaps someone noticed and wanted this behavior.

Setting the groupRowsBy input to null will immediately remove any information about grouped rows, including anything set by the groupedRows input. Previously, it may have been possible to set both groupedRows and groupRowsBy then remove the latter, retaining the original groupedRows. However, this seems like a strange use case.

Other information:
This addresses the question of how sorting should be handled when rows are grouped in only the most basic fashion. Grouped rows are always derived from the result of sorting the rows according to sorts. Therefore, changing the sort order of columns can change the order of groups since group order is determined by the position of the first row exhibiting that group trait.

A few alternate sort methods when rows are grouped:

  • Set groupRowsBy = null to disable row grouping when sorting on other columns (this PR makes this possible) so that the sort applies to all rows without groups.
  • Set externalSorting when sorting on other columns and sort rows with respect to the groups.
  • Set externalSorting = true and ignore sorting events to disable sorting

…nge to an on-demand invalidation model.

Fixes race conditions with the order in which inputs are set, allows application of sorting to grouped rows, enables removal of row grouping.
@amcdnl amcdnl requested a review from Hypercubed October 23, 2017 19:39
@lautarobock
Copy link

Any news about it?

@idpaterson
Copy link
Author

idpaterson commented Apr 10, 2018

@lautarobock, I had to move away from actual row grouping and just use a custom rowClass and sort to make certain records look and act like group headings. After working around the inability to sort I was faced with additional features that did not work quite as expected against grouped data. For example, paging applies to the number of groups on the page rather than the number of results and filtering results by a keyword can be extremely latent due to extra layout overhead when groups are enabled (in my case with non-debounced filtering the browser froze for about 800ms between each keystroke).

@lautarobock
Copy link

I've created another PR trying to solve the merge conflicts

@BenjaminBini
Copy link

hi guys, any news on this PR ?

@Guling85
Copy link

Any news on this guys?

@enavarro222
Copy link

Any news on this PR ?

@MateusSilva0101
Copy link

Alguma novidade sobre esse PR?

Any solution for this 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.

6 participants