refactor(sort): state should be determined and provided by container #15171
Conversation
googlebot
added
the
cla: yes
andrewseguin
added
the
target: patch
src/lib/sort/sort.ts
Outdated
| /** Whether to disable clearing the sorting state. */ | ||
| disableClear: boolean; | ||
| /** State provided to a `MatSortable`, includes the current `MatSort` active sortable. */ | ||
| export interface MatSortSortableState extends SortableState { |
There was a problem hiding this comment.
This name is a bit...repetitive.
There was a problem hiding this comment.
I agree, but it may be more valuable to keep it description. In the future there will be things like MatMultiSortSortableState.
Options:
MatSortableState/MatMultiSortableState
MatSortState/MatMultiSortState
src/lib/sort/sort.ts
Outdated
| _stateChanges: Observable<void>; | ||
|
|
||
| /** Registers the sortable so that it can be sorted. */ | ||
| register(sortable: MatSortable): void; |
There was a problem hiding this comment.
Looking over the code, I'm starting to now think we should move some of this to the cdk.
andrewseguin
added
target: major
andrewseguin
force-pushed
the
sort-refactor
branch
from
cfd9d03 to
a12b405
Compare
andrewseguin
force-pushed
the
sort-refactor
branch
5 times, most recently
from
2687434 to
898cc55
Compare
andrewseguin
force-pushed
the
sort-refactor
branch
from
898cc55 to
795a28f
Compare
This comment has been minimized.
This comment has been minimized.
|
@andrewseguin Will @crisbeto be able to review this PR soon, or should the review be passed to someone who does have time? |
|
This was de-prioritized behind our work on test harnesses and MDC integration. To move forward, we're still pending a final design discussion to make sure this is the right route for us to go. I'd love to get it in but unfortunately we're limited on resources |
|
@andrewseguin I ran into this while triaging PRs and found this old PR. Was this ever discussed / any updates? |
|
@wagnermaciel We wanted to figure out a solid design plan for this but unfortunately it just never got prioritized. We could discuss it as a team to decide if we want to move forward on it or just let it close |
andrewseguin
removed
the
cla: yes
|
Adding this PR as a reference in our backlog item for CDK Sort; closing until we have the effort prioritized |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Refactors
MatSortandMatSortHeaderso that they are not tightly coupled.Currently, the sort header determines its state by accessing properties from the
MatSortsuch asactiveanddirectionto determine if it is sorted. This means thatMatSortcannot be replaced by something like a multi-sort container.This change changes the interaction so that the
MatSortHeaderrequests its state from the container and there is a simple contract that the container needs to follow to be replaced.Note that this is the first of three phases needed to get us towards having a
MatMultiSort. The conversation can be found here #13538. Thanks to @relair for making the foundation for this feature