-
Notifications
You must be signed in to change notification settings - Fork 34
refactor: fully extend dynamic mat tables #1972
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
Conversation
…re visible for small screens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Remove the stray console.log in DynamicMatTableComponent to avoid leaking debug output into production logs.
- Ensure any manual RxJS subscriptions (e.g. dataSource.subscribe or sort.sortChange) are properly cleaned up in OnDestroy to prevent memory leaks.
- Clean up leftover virtual-scroll and tvsDataSource references in the directive/component now that only the standardDataSource is used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove the stray console.log in DynamicMatTableComponent to avoid leaking debug output into production logs.
- Ensure any manual RxJS subscriptions (e.g. dataSource.subscribe or sort.sortChange) are properly cleaned up in OnDestroy to prevent memory leaks.
- Clean up leftover virtual-scroll and tvsDataSource references in the directive/component now that only the standardDataSource is used.
## Individual Comments
### Comment 1
<location> `src/app/shared/modules/dynamic-material-table/table/dynamic-mat-table.component.ts:292` </location>
<code_context>
const visibleColumns = this.columns.filter(
(c) => c.display !== "hidden" && c.index < data.e.columnIndex,
);
+ console.log("DEBUG visibleColumns:", visibleColumns);
i = visibleColumns[visibleColumns.length - 1].index;
}
</code_context>
<issue_to_address>
Leftover debug logging should be removed for production code.
Console.log statements may expose sensitive data and clutter production logs. Please remove or use an appropriate logging solution.
</issue_to_address>
### Comment 2
<location> `src/app/shared/modules/dynamic-material-table/cores/table.core.directive.ts:356` </location>
<code_context>
- if (this.viewport) {
- this.viewport.scrollTo({ top: 0, behavior: "auto" });
+ if (this.tableContainer) {
+ this.tableContainer.nativeElement.scrollTop = 0;
}
this.tvsDataSource.clearData();
</code_context>
<issue_to_address>
Direct DOM manipulation for scrolling may not be cross-platform or framework-idiomatic.
Using nativeElement.scrollTop may cause compatibility issues in platforms where direct DOM access is discouraged. Prefer Angular's Renderer2 for safer, framework-consistent DOM manipulation.
Suggested implementation:
```typescript
public clear() {
if (!isNullorUndefined(this.tvsDataSource)) {
if (this.tableContainer) {
this.renderer.setProperty(this.tableContainer.nativeElement, 'scrollTop', 0);
}
this.tvsDataSource.clearData();
this.expandedElement = null;
this.cdr.detectChanges();
this.refreshColumn(this.tableColumns);
this.table.renderRows();
}
}
```
```typescript
this.tableSetting.scrollStrategy = value;
}
```
You must inject `Renderer2` in your directive/class constructor. For example:
```typescript
constructor(
private renderer: Renderer2,
// ...other injections
) { }
```
If your class does not already have Renderer2 injected, add it to the constructor and update all usages accordingly.
</issue_to_address>
### Comment 3
<location> `src/app/shared/modules/dynamic-material-table/cores/table-data-source.ts:21` </location>
<code_context>
- T extends TableRow,
-> extends MatTableDataSource<T> {
- private streamsReady: boolean;
+export class TableDataSource<T extends TableRow> extends MatTableDataSource<T> {
private filterMap: HashMap<AbstractFilter[]> = {};
- public dataToRender$: Subject<T[]>;
</code_context>
<issue_to_address>
Consider maintaining the separation of filtering logic and virtual scroll features to keep responsibilities clear and avoid code duplication.
No actionable changes—this split cleanly separates core filtering (in TableDataSource) from virtual‐scroll concerns (in TableVirtualScrollDataSource) without duplication.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/app/shared/modules/dynamic-material-table/table/dynamic-mat-table.component.ts
Outdated
Show resolved
Hide resolved
if (this.viewport) { | ||
this.viewport.scrollTo({ top: 0, behavior: "auto" }); | ||
if (this.tableContainer) { | ||
this.tableContainer.nativeElement.scrollTop = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Direct DOM manipulation for scrolling may not be cross-platform or framework-idiomatic.
Using nativeElement.scrollTop may cause compatibility issues in platforms where direct DOM access is discouraged. Prefer Angular's Renderer2 for safer, framework-consistent DOM manipulation.
Suggested implementation:
public clear() {
if (!isNullorUndefined(this.tvsDataSource)) {
if (this.tableContainer) {
this.renderer.setProperty(this.tableContainer.nativeElement, 'scrollTop', 0);
}
this.tvsDataSource.clearData();
this.expandedElement = null;
this.cdr.detectChanges();
this.refreshColumn(this.tableColumns);
this.table.renderRows();
}
}
this.tableSetting.scrollStrategy = value;
}
You must inject Renderer2
in your directive/class constructor. For example:
constructor(
private renderer: Renderer2,
// ...other injections
) { }
If your class does not already have Renderer2 injected, add it to the constructor and update all usages accordingly.
T extends TableRow, | ||
> extends MatTableDataSource<T> { | ||
private streamsReady: boolean; | ||
export class TableDataSource<T extends TableRow> extends MatTableDataSource<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider maintaining the separation of filtering logic and virtual scroll features to keep responsibilities clear and avoid code duplication.
No actionable changes—this split cleanly separates core filtering (in TableDataSource) from virtual‐scroll concerns (in TableVirtualScrollDataSource) without duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this version on my end and I have few concerns. First it is very hard to see the data in the table as there is no horizontal scroll like it used to be and if I expand some columns some others get invisible and the the one can not see anything from the data. I think the previous version worked much better in that sense. Another one is that in the proposals table we have a double horizontal scroll for some reason. Here is a recording that can explain the problems better:
Screenshare.-.2025-08-21.1_58_28.PM.mp4
src/app/shared/modules/dynamic-material-table/cores/table-data-source.ts
Show resolved
Hide resolved
src/app/shared/modules/dynamic-material-table/table/dynamic-mat-table.component.html
Show resolved
Hide resolved
src/app/shared/modules/dynamic-material-table/table/dynamic-mat-table.component.ts
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now
Description
Dynamic mat tables are now fully extended and pagination system has also been added above the tables.
Motivation
Background on use case, changes needed
Fixes:
Changes:
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Refactor dynamic-mat-table to fully migrate from virtual scrolling to a standard Material table configuration with built-in pagination and a custom TableDataSource, clean up related code and templates, update styles, and adjust end-to-end tests accordingly.
New Features:
Enhancements:
Tests: