From 627d64dffe1c015b3f50af27ee5ef9cc17501a27 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Sun, 21 Apr 2024 18:08:34 +0900 Subject: [PATCH 1/4] Memoizes column header functions --- .../webapp/metrics/views/card_renderer/BUILD | 1 + .../scalar_card_data_table.ng.html | 4 +- .../card_renderer/scalar_card_data_table.ts | 27 +++++++---- .../webapp/runs/views/runs_table/BUILD | 1 + .../views/runs_table/runs_data_table.ng.html | 4 +- .../runs/views/runs_table/runs_data_table.ts | 47 +++++++++---------- 6 files changed, 47 insertions(+), 37 deletions(-) diff --git a/tensorboard/webapp/metrics/views/card_renderer/BUILD b/tensorboard/webapp/metrics/views/card_renderer/BUILD index 0790e5e8f3..288f052416 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/BUILD +++ b/tensorboard/webapp/metrics/views/card_renderer/BUILD @@ -286,6 +286,7 @@ tf_ng_module( ":utils", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/runs:types", + "//tensorboard/webapp/util:memoize", "//tensorboard/webapp/widgets/card_fob:types", "//tensorboard/webapp/widgets/data_table", "//tensorboard/webapp/widgets/data_table:types", diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html index 880ce02d9c..f787fb2a85 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html @@ -30,7 +30,7 @@ (loadAllColumns)="loadAllColumns.emit()" > - + - + ).concat( + [ + { + name: 'color', + displayName: '', + type: ColumnHeaderType.COLOR, + enabled: true, + }, + ], + headers + ); } getMinPointInRange( diff --git a/tensorboard/webapp/runs/views/runs_table/BUILD b/tensorboard/webapp/runs/views/runs_table/BUILD index 719cb0bd5b..f425ef4b20 100644 --- a/tensorboard/webapp/runs/views/runs_table/BUILD +++ b/tensorboard/webapp/runs/views/runs_table/BUILD @@ -107,6 +107,7 @@ tf_ng_module( "//tensorboard/webapp/types:ui", "//tensorboard/webapp/util:colors", "//tensorboard/webapp/util:matcher", + "//tensorboard/webapp/util:memoize", "//tensorboard/webapp/widgets/custom_modal", "//tensorboard/webapp/widgets/data_table", "//tensorboard/webapp/widgets/data_table:filter_dialog", diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html index 83688127bd..41cf95f2a0 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html @@ -41,7 +41,7 @@ (loadAllColumns)="loadAllColumns.emit()" > - + - + (); @Output() loadAllColumns = new EventEmitter(); - // These columns must be stored and reused to stop needless re-rendering of - // the content and headers in these columns. This has been known to cause - // problems with the controls in these columns, specifically the color picker. - beforeColumns = [ - { - name: 'selected', - displayName: '', - type: ColumnHeaderType.CUSTOM, - enabled: true, - }, - ]; + // Columns must be memoized to stop needless re-rendering of the content and headers in these + // columns. This has been known to cause problems with the controls in these columns, + // specifically the add button. + extendHeaders = memoize(this.internalExtendHeaders); - afterColumns = [ - { - name: 'color', - displayName: '', - type: ColumnHeaderType.COLOR, - enabled: true, - }, - ]; - - getHeaders() { + private internalExtendHeaders(headers: ColumnHeader[]) { return ([] as Array).concat( - this.beforeColumns, - this.headers, - this.afterColumns + [ + { + name: 'selected', + displayName: '', + type: ColumnHeaderType.CUSTOM, + enabled: true, + }, + ], + headers, + [ + { + name: 'color', + displayName: '', + type: ColumnHeaderType.COLOR, + enabled: true, + }, + ] ); } From 24d55042f337276c3ec9039ac5600f9d7619e364 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Sun, 21 Apr 2024 18:41:05 +0900 Subject: [PATCH 2/4] Fixes test --- .../runs/views/runs_table/runs_data_table_test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts b/tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts index f2b5432481..6f5d9c411b 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts @@ -220,25 +220,25 @@ describe('runs_data_table', () => { }); }); - // If the call to getHeaders produces a new color header on each call, then + // If the call to extendHeaders produces a new color header on each call, then // the content gets re-rendered when the color picker opens. This causes the // color picker modal to close immediately. This test ensures that we keep // the same reference to the color header and continue to use that reference // instead of creating a new one on each call. - it('getHeaders does not produce new color header on each call', () => { + it('extendHeaders does not produce new color header on each call', () => { const fixture = createComponent({}); - const runsDataTable = fixture.debugElement.query( By.directive(RunsDataTable) ); + const {headers} = runsDataTable.componentInstance; expect( runsDataTable.componentInstance - .getHeaders() + .extendHeaders(headers) .find((header: ColumnHeader) => header.name === 'color') ).toBe( runsDataTable.componentInstance - .getHeaders() + .extendHeaders(headers) .find((header: ColumnHeader) => header.name === 'color') ); }); From e11681dd158fe0e35bc7a25dea5d44d389dd69d5 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 22 Apr 2024 14:14:29 +0900 Subject: [PATCH 3/4] Moves expand button to layout container and fixes sidebar scroll --- .../webapp/core/views/layout_container.scss | 72 +++++++++++++++---- .../webapp/core/views/layout_container.ts | 28 +++++++- tensorboard/webapp/core/views/layout_test.ts | 2 +- .../runs_selector/runs_selector_component.ts | 7 ++ .../views/runs_table/runs_data_table.ng.html | 13 ---- .../views/runs_table/runs_data_table.scss | 54 ++------------ .../runs/views/runs_table/runs_data_table.ts | 2 - .../views/runs_table/runs_table_container.ts | 9 --- 8 files changed, 95 insertions(+), 92 deletions(-) diff --git a/tensorboard/webapp/core/views/layout_container.scss b/tensorboard/webapp/core/views/layout_container.scss index 57a197257e..8b437f2510 100644 --- a/tensorboard/webapp/core/views/layout_container.scss +++ b/tensorboard/webapp/core/views/layout_container.scss @@ -24,18 +24,34 @@ limitations under the License. .sidebar { max-width: 80vw; + position: relative; } .resizer, -.expand { +.expand-collapsed-sidebar { @include tb-theme-foreground-prop(border-color, border); box-sizing: border-box; flex: 0 0; justify-self: stretch; } -.expand { +.expand-collapsed-sidebar { width: 20px; + align-items: center; + background: transparent; + border-style: solid; + border-width: 0 1px 0 0; + color: inherit; + contain: content; + cursor: pointer; + display: flex; + justify-self: stretch; + padding: 0; + + mat-icon { + transform: rotate(-90deg); + transform-origin: center; + } } .resizer { @@ -63,20 +79,46 @@ limitations under the License. } } -.expand { - align-items: center; - background: transparent; - border-style: solid; - border-width: 0 1px 0 0; - color: inherit; - contain: content; - cursor: pointer; +.full-screen-toggle { + opacity: 0; + position: absolute; + height: 100%; + // Ensure the button is on the right side then add 2px for the drag target. + left: calc(100% + 2px); + top: 0; + z-index: 1; display: flex; - justify-self: stretch; - padding: 0; + align-items: center; - mat-icon { - transform: rotate(-90deg); - transform-origin: center; + &:hover { + opacity: 0.8; + } + + &.full-screen { + left: unset; + right: 0; + } + + .full-screen-btn { + $_arrow_size: 16px; + $_arrow_button_size: calc($_arrow_size + 4px); + + background-color: gray; + padding: 0; + min-width: $_arrow_button_size; + width: $_arrow_button_size; + + &.expand { + border-radius: 0 $_arrow_button_size $_arrow_button_size 0; + } + + &.collapse { + border-radius: $_arrow_button_size 0 0 $_arrow_button_size; + } + + .expand-collapse-icon { + font-size: $_arrow_size; + margin-right: 0; // Removes default + } } } diff --git a/tensorboard/webapp/core/views/layout_container.ts b/tensorboard/webapp/core/views/layout_container.ts index e90276da5b..f48e6303f8 100644 --- a/tensorboard/webapp/core/views/layout_container.ts +++ b/tensorboard/webapp/core/views/layout_container.ts @@ -22,7 +22,7 @@ import {Store} from '@ngrx/store'; import {fromEvent, Observable, Subject} from 'rxjs'; import {combineLatestWith, filter, map, takeUntil} from 'rxjs/operators'; import {MouseEventButtons} from '../../util/dom'; -import {sideBarWidthChanged} from '../actions'; +import {runsTableFullScreenToggled, sideBarWidthChanged} from '../actions'; import {State} from '../state'; import { getRunsTableFullScreen, @@ -34,7 +34,7 @@ import { template: ` +
{ let dispatchedActions: Action[] = []; const byCss = { - EXPANDER: By.css('.expand'), + EXPANDER: By.css('.expand-collapsed-sidebar'), RESIZER: By.css('.resizer'), SIDEBAR_CONTAINER: By.css('nav'), LAYOUT: By.directive(LayoutContainer), diff --git a/tensorboard/webapp/runs/views/runs_selector/runs_selector_component.ts b/tensorboard/webapp/runs/views/runs_selector/runs_selector_component.ts index eb230a3546..db7d189a72 100644 --- a/tensorboard/webapp/runs/views/runs_selector/runs_selector_component.ts +++ b/tensorboard/webapp/runs/views/runs_selector/runs_selector_component.ts @@ -25,6 +25,13 @@ import {RunsTableColumn} from '../runs_table/types'; `, styles: [ ` + :host { + display: block; + height: 100%; + width: 100%; + overflow: auto; + } + runs-table { height: 100%; } diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html index 41cf95f2a0..1b3dc0416a 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html @@ -106,16 +106,3 @@
-
- -
diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.scss b/tensorboard/webapp/runs/views/runs_table/runs_data_table.scss index 246b95cbe2..acfce22fa1 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.scss +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.scss @@ -18,12 +18,11 @@ $_circle-size: 20px; $_arrow_size: 16px; :host { - width: 100%; -} + min-width: 100%; -:host { - overflow-y: auto; - width: 100%; + & ::ng-deep tb-data-table .data-table { + @include tb-theme-foreground-prop(border-top, border, 1px solid); + } } .color-container { @@ -72,49 +71,7 @@ tb-data-table-header-cell:last-of-type { width: 40px; } -.full-screen-toggle { - opacity: 0; - position: absolute; - height: 100%; - // Ensure the button is on the right side then add 2px for the drag target. - left: calc(100% + 2px); - top: 0; - z-index: 1; - display: flex; - align-items: center; - - &:hover { - opacity: 0.8; - } - - &.full-screen { - left: unset; - right: 0; - } - - .full-screen-btn { - background-color: gray; - padding: 0; - min-width: $_arrow_size; - width: $_arrow_size; - - &.expand { - border-radius: 0 $_arrow_size $_arrow_size 0; - } - - &.collapse { - border-radius: $_arrow_size 0 0 $_arrow_size; - } - - .expand-collapse-icon { - font-size: $_arrow_size; - width: $_arrow_size; - } - } -} - .filter-row { - @include tb-theme-foreground-prop(border-bottom, border, 1px solid); display: flex; align-items: center; height: 48px; @@ -125,6 +82,3 @@ tb-data-table-header-cell:last-of-type { flex-grow: 1; } } -.table-container { - overflow-x: auto; -} diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts index 95a1a2f4aa..57e75098a4 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts @@ -43,7 +43,6 @@ export class RunsDataTable { @Input() sortingInfo!: SortingInfo; @Input() experimentIds!: string[]; @Input() regexFilter!: string; - @Input() isFullScreen!: boolean; @Input() selectableColumns!: ColumnHeader[]; @Input() numColumnsLoaded!: number; @Input() numColumnsToLoad!: number; @@ -57,7 +56,6 @@ export class RunsDataTable { @Output() onSelectionToggle = new EventEmitter(); @Output() onAllSelectionToggle = new EventEmitter(); @Output() onRegexFilterChange = new EventEmitter(); - @Output() toggleFullScreen = new EventEmitter(); @Output() onRunColorChange = new EventEmitter<{ runId: string; newColor: string; diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts index 375b9c9391..bac141cbc5 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts @@ -45,7 +45,6 @@ import { getRuns, getRunSelectorRegexFilter, getRunsLoadState, - getRunsTableFullScreen, getRunsTableSortingInfo, getGroupedRunsTableHeaders, } from '../../../selectors'; @@ -73,7 +72,6 @@ import { getFilteredRenderableRuns, getSelectableColumns, } from '../../../metrics/views/main_view/common_selectors'; -import {runsTableFullScreenToggled} from '../../../core/actions'; import {sortTableDataItems} from './sorting_utils'; const getRunsLoading = createSelector< @@ -102,7 +100,6 @@ const getRunsLoading = createSelector< [sortingInfo]="sortingInfo$ | async" [experimentIds]="experimentIds" [regexFilter]="regexFilter$ | async" - [isFullScreen]="runsTableFullScreen$ | async" [loading]="loading$ | async" (sortDataBy)="sortDataBy($event)" (orderColumns)="orderColumns($event)" @@ -111,7 +108,6 @@ const getRunsLoading = createSelector< (onRunColorChange)="onRunColorChange($event)" (onRegexFilterChange)="onRegexFilterChange($event)" (onSelectionDblClick)="onRunSelectionDblClick($event)" - (toggleFullScreen)="toggleFullScreen()" (addColumn)="addColumn($event)" (removeColumn)="removeColumn($event)" (addFilter)="addHparamFilter($event)" @@ -147,7 +143,6 @@ export class RunsTableContainer implements OnInit, OnDestroy { regexFilter$ = this.store.select(getRunSelectorRegexFilter); runsColumns$ = this.store.select(getGroupedRunsTableHeaders); - runsTableFullScreen$ = this.store.select(getRunsTableFullScreen); selectableColumns$ = this.store.select(getSelectableColumns); numColumnsLoaded$ = this.store.select( hparamsSelectors.getNumDashboardHparamsLoaded @@ -328,10 +323,6 @@ export class RunsTableContainer implements OnInit, OnDestroy { this.store.dispatch(runColorChanged({runId, newColor})); } - toggleFullScreen() { - this.store.dispatch(runsTableFullScreenToggled()); - } - addColumn({column, nextTo, side}: AddColumnEvent) { this.store.dispatch( hparamsActions.dashboardHparamColumnAdded({ From 781e9800c0151c8ae30d1408af820bd5846257c5 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Wed, 24 Apr 2024 23:30:30 +0900 Subject: [PATCH 4/4] Remove unused css variable --- tensorboard/webapp/runs/views/runs_table/runs_data_table.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.scss b/tensorboard/webapp/runs/views/runs_table/runs_data_table.scss index acfce22fa1..2f1e9610c1 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.scss +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.scss @@ -15,7 +15,6 @@ limitations under the License. @use '@angular/material' as mat; @import 'tensorboard/webapp/theme/tb_theme'; $_circle-size: 20px; -$_arrow_size: 16px; :host { min-width: 100%;