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(MatSort): Enable multi-sorting capability on current Interface #24102

Open
imerljak opened this issue Dec 14, 2021 · 7 comments · May be fixed by #28458
Open

feat(MatSort): Enable multi-sorting capability on current Interface #24102

imerljak opened this issue Dec 14, 2021 · 7 comments · May be fixed by #28458
Labels
area: material/sort feature This issue represents a new feature or feature request rather than a bug or bug fix P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@imerljak
Copy link

Feature Description

Currently MatSort has a simplistic design that enables sorting only on one column at a time.

Most of enterprise level applications should need multi-sorting capabilities on its datasets. And having it included would be really nice.

In simple terms, my proposal would be to expose a boolean input matMultiSort flag and refactor MatSort and MatSortHeader so that MatSort will hold the MatSortable's state, and expose isActive(id: string) => boolean and getNextDirection(id: string) => SortDirection that will be used instead of directly accessing active and direction.

MatSort will hold a sortState: Map<string, Sort> that can be used to query its state whenever a sort event occurs.

This change should avoid most (if not all) compatibility issues. But it would be best if we can deprecate the current sortChanged: EventEmitter<Sort> for one that emits Sort[] instead, this way we wont need to expose sortState (although most of the usage examples for this API expect one to access its internal state anyway).

Known issues

This change of sorting behavior will require a review of the included sorting method for MatTableDataSource, that was designed around a single column sort.
Having multiple possible sortable columns will require a better strategy on sorting (weighting by index position?).
Or, whenever the MatTableDataSource default implementation is used, multi-sorting could be disabled?

Use Case

We want to be able to enable sorting based on multiple columns at a time.

This should be done with minimal changes.

Proposed usage:

<!-- Adding matSortMultiple will enable multi-sorting -->
<table mat-table matSort matSortMultiple matSortActive="position" matSortDirection="asc" [dataSource]="dataSource">
    <ng-container matColumnDef="name">
    <th mat-header-cell *matHeaderCellDef mat-sort-header sortActionDescription="Sort by name">
      Name
    </th>
    <td mat-cell *matCellDef="let element"> {{element.name}} </td>
  </ng-container>

<ng-container matColumnDef="symbol">
    <th mat-header-cell *matHeaderCellDef mat-sort-header sortActionDescription="Sort by symbol">
      Symbol
    </th>
    <td mat-cell *matCellDef="let element"> {{element.symbol}} </td>
  </ng-container>
</table>
@ViewChild(MatSort) matSort: MatSort;

ngAfterViewInit() {
    matSort.sortChanged.pipe(map(() => Array.from(matSort.sortstate.values()))
    .subscribe((sorts: Sort[])=> {
        // do something with the Sort[] 
    });
}
@imerljak imerljak added feature This issue represents a new feature or feature request rather than a bug or bug fix needs triage This issue needs to be triaged by the team labels Dec 14, 2021
imerljak added a commit to imerljak/components that referenced this issue Dec 14, 2021
Adds multi-sorting capability to MatSort, enables sorting on multiple table columns at once.
Feature is activated by using matSortMultiple property on table tag.

BREAKING CHANGE:

`MatSort.active` and `MatSort.direction` wont be source of truth for current sorted but as a
reference for the default sorted column and its direction. So any inner usage of it should instead
reference `MatSort.isActive` and `MatSort.getCurrentSortDirection` respectively.
For user code, we should use `MatSort.sortState` that holds the complete sort state.

Fixes angular#24102
@andrewseguin andrewseguin added area: material/sort P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels Jan 5, 2022
@angular-robot
Copy link
Contributor

angular-robot bot commented Mar 14, 2022

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

@angular-robot
Copy link
Contributor

angular-robot bot commented Apr 3, 2022

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

@dzaunerOci
Copy link

Is it possible to start the voting process again, this feature would be very helpful.

@zarend
Copy link
Contributor

zarend commented Jan 2, 2024

Hello @imerljak,

Would you like to send us a PR for this? This feature request is open source, and anyone is welcome to work on it.

We have a requirement to be backwards compatible, so any PR would need to add multi-sorting without breaking the existing "single-sorting". If the PR changes the sort behavior when not multi-sorting, then we might need an opt-out or opt-in.

Best Regards,

Zach

@imerljak
Copy link
Author

imerljak commented Jan 2, 2024

Hello @imerljak,

Would you like to send us a PR for this? This feature request is open source, and anyone is welcome to work on it.

We have a requirement to be backwards compatible, so any PR would need to add multi-sorting without breaking the existing "single-sorting". If the PR changes the sort behavior when not multi-sorting, then we might need an opt-out or opt-in.

Best Regards,

Zach

I have the required changes at my cloned repo. But will need to checkout if it is still working as expected since it has been some time already.

Will find some time to rebase it and run the tests again to see if it still works and open a PR.

If I recall correctly it is indeed backwards compatible. Or very close to.

Can't commit to a date though but will try to look into it as soon as possible.

@zarend
Copy link
Contributor

zarend commented Jan 2, 2024

No problem, let us know if you have questions.

imerljak pushed a commit to imerljak/components that referenced this issue Jan 21, 2024
Adds multi-column sorting capability to MatSort, allowing to sort a table on multiple of its
columns at once by toggling matSortMultiple.

This feature adds a new sortState variable inside MatSort that should be used as a source of truth
when matSortMultiple is enabled.
Fixes angular#24102
@imerljak imerljak linked a pull request Jan 21, 2024 that will close this issue
imerljak added a commit to imerljak/components that referenced this issue Jan 21, 2024
Adds multi-column sorting capability to MatSort, allowing to sort a table on multiple of its
columns at once by toggling matSortMultiple.

This feature adds a new sortState variable inside MatSort that should be used as a source of truth
when matSortMultiple is enabled.
Fixes angular#24102
@imerljak
Copy link
Author

imerljak commented Jan 21, 2024

@zarend Opened the MR with the changes for adding multi-sort.

Reviewed my previous implementation in order to avoid any kind of conflicts with current sorting behaviour. So, unless we want to keep track of the current internal sorting state, just toggling the multi-sort flag is enough to make it work.

From what I was able to test on the sorting algorithm it is working as expected, and the behaviour is exactly the same as we have in this other lib which I used as a comparison. For both sorting on multiple columns, as with the order in which we toggle the sorting and cycle through it.

No fancy shift metakey though.. didn't think it was needed atm.

Waiting for feedback over there.. #28458

imerljak added a commit to imerljak/components that referenced this issue Jan 21, 2024
Adds multi-column sorting capability to MatSort, allowing to sort a table on multiple of its
columns at once by toggling matSortMultiple.

This feature adds a new sortState variable inside MatSort that should be used as a source of truth
when matSortMultiple is enabled.
Fixes angular#24102
imerljak added a commit to imerljak/components that referenced this issue Mar 25, 2024
Adds multi-column sorting capability to MatSort, allowing to sort a table on multiple of its
columns at once by toggling matSortMultiple.

This feature adds a new sortState variable inside MatSort that should be used as a source of truth
when matSortMultiple is enabled.
Fixes angular#24102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: material/sort feature This issue represents a new feature or feature request rather than a bug or bug fix P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants