rfellows commented on code in PR #10523: URL: https://github.com/apache/nifi/pull/10523#discussion_r2523661477
########## nifi-frontend/src/main/frontend/apps/nifi/src/app/ui/common/component-state/component-state.component.scss: ########## Review Comment: * the effective hover color of the rows doesn't match the other tables. the summary tables, for instance, this is the applied style: https://github.com/apache/nifi/blob/main/nifi-frontend/src/main/frontend/libs/shared/src/assets/styles/_listing-table.scss#L93-L102 * when not in clustered mode, the sizing/placement of the columns isn't good: <img width="996" height="340" alt="Screenshot 2025-11-13 at 09 25 32" src="https://github.com/user-attachments/assets/fc038bdd-af16-4561-992c-3a45cdf24559"; /> * Dark mode isn't accounted for fully. there are lines separating the rows. <img width="1040" height="659" alt="Screenshot 2025-11-13 at 09 24 15" src="https://github.com/user-attachments/assets/58215ef6-b4dd-40bb-995d-81affe26b9c2"; /> ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/ui/common/component-state/component-state.component.scss: ########## @@ -21,10 +21,75 @@ @include mat.button-density(-1); .listing-table { - table { - .mat-column-key { - width: 200px; + @include mat.table-density(-4); + + .table-container { + display: flex; + flex-direction: column; + } + + .header-wrapper { + padding-right: 15px; // Account for scrollbar width + background-color: var(--mat-sys-secondary); + border-bottom-width: 1px; + border-bottom-style: solid; + } + + .cdk-table-virtual { + width: 100%; + table-layout: fixed; + + thead { + display: table; + width: 100%; + table-layout: fixed; + + tr { + height: 40px; + } + + th { + height: 40px; + color: var(--mat-sys-on-secondary); + border-bottom-width: 0; // Override global listing-table border + + &:first-child { + width: 200px; + } + + &:last-child { + width: 52px; + } + } } + + tbody { + display: table; + width: 100%; + table-layout: fixed; + + tr { + height: 36px; // Must match ROW_HEIGHT constant + } + + td { + height: 36px; // Must match ROW_HEIGHT constant + + &:first-child { + width: 200px; + } + + &:last-child { + width: 52px; + } + } + } + } + + .table-body-viewport { + flex: 1; + overflow-y: scroll; // Always show scrollbar for consistent width Review Comment: I understand why we do this here, just not a fan of it. ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/ui/common/component-state/component-state.component.scss: ########## @@ -21,10 +21,75 @@ @include mat.button-density(-1); .listing-table { - table { - .mat-column-key { - width: 200px; + @include mat.table-density(-4); + + .table-container { + display: flex; + flex-direction: column; + } + + .header-wrapper { + padding-right: 15px; // Account for scrollbar width Review Comment: scrollbars have different widths on different platforms. mac is 15 in the big browsers, but on windows i'm prettey certain it is 17. ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/ui/common/component-state/component-state.component.html: ########## @@ -71,64 +71,72 @@ <h2 mat-dialog-title>Component State</h2> </div> </div> <div class="listing-table flex-1 relative"> - <div class="absolute inset-0 overflow-y-auto overflow-x-hidden"> - <table - mat-table - [dataSource]="dataSource" - matSort - matSortDisableClear - (matSortChange)="sortData($event)" - [matSortActive]="initialSortColumn" - [matSortDirection]="initialSortDirection"> - <!-- Key Column --> - <ng-container matColumnDef="key"> - <th mat-header-cell *matHeaderCellDef mat-sort-header>Key</th> - <td mat-cell *matCellDef="let item"> - {{ item.key }} - </td> - </ng-container> - - <!-- Value Column --> - <ng-container matColumnDef="value"> - <th mat-header-cell *matHeaderCellDef mat-sort-header>Value</th> - <td mat-cell *matCellDef="let item" [title]="item.value"> - {{ item.value }} - </td> - </ng-container> - - <!-- Scope Column --> - @if (displayedColumns.includes('scope')) { - <ng-container matColumnDef="scope"> - <th mat-header-cell *matHeaderCellDef mat-sort-header>Scope</th> - <td mat-cell *matCellDef="let item" [title]="item.scope"> - {{ item.scope }} - </td> - </ng-container> - } - - <!-- Actions Column --> - @if (displayedColumns.includes('actions')) { - <ng-container matColumnDef="actions"> - <th mat-header-cell *matHeaderCellDef></th> - <td mat-cell *matCellDef="let item"> - <button - [disabled]="isClearing" - mat-icon-button - color="primary" - (click)="clearComponentStateEntry(item)" - [title]="'Clear state entry: ' + item.key"> - <i class="fa fa-trash"></i> - </button> - </td> - </ng-container> - } - - <tr mat-header-row *matHeaderRowDef="displayedColumns; sticky: true"></tr> - <tr - mat-row - *matRowDef="let row; let even = even; columns: displayedColumns" - [class.even]="even"></tr> - </table> + <div class="absolute inset-0 table-container"> + <div class="header-wrapper"> + <table class="mat-mdc-table mdc-data-table__table cdk-table-virtual"> + <thead> + <tr + class="mat-mdc-header-row mdc-data-table__header-row" + matSort + matSortDisableClear + (matSortChange)="sortData($event)" + [matSortActive]="currentSortColumn" + [matSortDirection]="currentSortDirection"> + <th + class="mat-mdc-header-cell mdc-data-table__header-cell" + mat-sort-header="key"> + Key + </th> + <th + class="mat-mdc-header-cell mdc-data-table__header-cell" + mat-sort-header="value"> + Value + </th> + @if (displayedColumns.includes('scope')) { + <th + class="mat-mdc-header-cell mdc-data-table__header-cell" + mat-sort-header="scope"> + Scope + </th> + } + @if (displayedColumns.includes('actions')) { + <th class="mat-mdc-header-cell mdc-data-table__header-cell"></th> + } + </tr> + </thead> + </table> + </div> + <cdk-virtual-scroll-viewport [itemSize]="ROW_HEIGHT" class="table-body-viewport"> + <table class="mat-mdc-table mdc-data-table__table cdk-table-virtual"> + <tbody> + <tr + *cdkVirtualFor="let item of dataSource; trackBy: trackByKey" + class="mat-mdc-row mdc-data-table__row data-row"> Review Comment: Note: we aren't indicating even/odd rows. I realize that we don't colorize them anymore but the other tables support it, if we decide to bring that even/odd coloring back this would not "automatically" get those styles. ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/ui/common/component-state/component-state.component.ts: ########## @@ -189,12 +206,51 @@ export class ComponentStateDialog extends CloseOnEscapeDialog implements AfterVi } applyFilter(filterTerm: string) { - this.dataSource.filter = filterTerm.trim().toLowerCase(); - this.filteredEntries = this.dataSource.filteredData.length; + const term = filterTerm.trim().toLowerCase(); + + if (!term) { + // No filter - show all items + this.dataSource = this.allStateItems; + this.filteredEntries = this.allStateItems.length; + } else { + // Filter the full dataset + const filtered = this.allStateItems.filter( + (item) => + item.key.toLowerCase().includes(term) || + item.value.toLowerCase().includes(term) || + (item.scope && item.scope.toLowerCase().includes(term)) + ); + this.dataSource = filtered; + this.filteredEntries = filtered.length; + } } sortData(sort: Sort) { - this.dataSource.data = this.sortStateItems(this.dataSource.data, sort); + this.currentSortColumn = sort.active as 'key' | 'value' | 'scope'; + this.currentSortDirection = sort.direction as 'asc' | 'desc'; + + // Determine what data to sort (filtered or all) + const filterTerm = this.filterForm.get('filterTerm')?.value?.trim().toLowerCase(); + let dataToSort = this.allStateItems; + + if (filterTerm) { + dataToSort = this.allStateItems.filter( + (item) => + item.key.toLowerCase().includes(filterTerm) || + item.value.toLowerCase().includes(filterTerm) || + (item.scope && item.scope.toLowerCase().includes(filterTerm)) + ); Review Comment: we do this same logic in the applyFilter method. might make sense to extract it into its own method for re-use in both places. ########## nifi-frontend/src/main/frontend/apps/nifi/src/app/ui/common/component-state/component-state.component.ts: ########## @@ -74,7 +76,15 @@ export class ComponentStateDialog extends CloseOnEscapeDialog implements AfterVi clearing: Signal<boolean> = this.store.selectSignal(selectClearing); displayedColumns: string[] = ['key', 'value']; - dataSource: MatTableDataSource<StateItem> = new MatTableDataSource<StateItem>(); + dataSource: StateItem[] = []; + allStateItems: StateItem[] = []; // Full dataset for filtering + + // Virtual scroll configuration + readonly ROW_HEIGHT = 36; // Height of each table row in pixels (density -4) + + // Sort state + currentSortColumn: 'key' | 'value' | 'scope' = 'key'; + currentSortDirection: 'asc' | 'desc' = 'asc'; Review Comment: probably should initialize these to the initial sort inputs ```suggestion currentSortColumn: 'key' | 'value' | 'scope' = this.initialSortColumn; currentSortDirection: 'asc' | 'desc' = this.initialSortDirection; ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
