fix: improve types for row grouping #81
Conversation
spike-rabbit
force-pushed
the
fix/improve-types-remaining
branch
2 times, most recently
from
1d218f2
to
090d833
Compare
| @@ -86,8 +86,7 @@ export class DataTableRowWrapperComponent<TRow = any> implements DoCheck, OnInit | |||
|
|
|||
| @Input() set rowIndex(val: number) { | |||
| this._rowIndex = val; | |||
| this.rowContext.rowIndex = val; | |||
| this.groupContext.rowIndex = val; | |||
| (this.rowContext ?? this.groupContext).rowIndex = val; | |||
There was a problem hiding this comment.
I think we should merge group and row context types and also merge the two properties.
There was a problem hiding this comment.
I am against merging. Those types just share two properties. The others are different.
export interface GroupContext<TRow> {
group: Group<TRow>;
expanded: boolean;
rowIndex: number;
}
export interface RowDetailContext<TRow> {
row: TRow;
expanded: boolean;
rowIndex: number;
disableRow$?: Observable<boolean>;
}They are emitted by different directives (DatatableGroupHeaderTemplateDirective and DatatableRowDetailTemplateDirective) so merging them would make them bad as we have properties in there which are never used in certain context.
Sure, the code looks bad this way, but we need to find a better solution in another MR. Merging the interface is far more worse in my opinion as this has a negative impact on consumer.
| @@ -99,7 +98,7 @@ export class DataTableRowWrapperComponent<TRow = any> implements DoCheck, OnInit | |||
|
|
|||
| @Input() set expanded(val: boolean) { | |||
| this._expanded = val; | |||
| this.groupContext.expanded = val; | |||
| (this.groupContext ?? this.rowContext)!.expanded = val; | |||
There was a problem hiding this comment.
Merge into one property if possible and make it protected.
| groupContext?: GroupContext<TRow>; | ||
| rowContext?: RowDetailContext<TRow>; |
| @@ -14,7 +14,7 @@ export interface HeaderCellContext { | |||
| } | |||
|
|
|||
| export interface GroupContext<TRow> { | |||
There was a problem hiding this comment.
merge with type RowDetailContext
| value: Group<TRow>; | ||
| } | ||
|
|
||
| export interface AllGroupsToggleEvent { |
There was a problem hiding this comment.
I find not usage of this interface. Why do we need it?
There was a problem hiding this comment.
It is part of the GroupToggleEvents . The AllGroupsToggleEvent type is indirectly used in DatatableGroupHeaderDirective#expandAllGroups
|
@spike-rabbit Back to you :-) |
spike-rabbit
force-pushed
the
fix/improve-types-remaining
branch
2 times, most recently
from
1619e28
to
0446116
Compare
|
@spike-rabbit I think the body component should also make use of the new toggle types. |
spike-rabbit
force-pushed
the
fix/improve-types-remaining
branch
from
0446116
to
5f37451
Compare
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: