From bf999efaf8559b4c4bd89702ed72064ab49f2116 Mon Sep 17 00:00:00 2001 From: Brenda Huang Date: Tue, 30 May 2023 03:52:29 +0000 Subject: [PATCH 01/12] add delete button with functionality --- tensorboard/webapp/metrics/actions/index.ts | 6 +++ tensorboard/webapp/metrics/internal_types.ts | 6 +++ .../webapp/metrics/store/metrics_reducers.ts | 40 +++++++++++++++++++ .../scalar_card_component.ng.html | 1 + .../card_renderer/scalar_card_component.ts | 15 ++++++- .../card_renderer/scalar_card_container.ts | 8 +++- .../card_renderer/scalar_card_data_table.ts | 8 ++++ .../data_table/data_table_component.ng.html | 8 +++- .../data_table/data_table_component.ts | 14 +++++++ 9 files changed, 103 insertions(+), 3 deletions(-) diff --git a/tensorboard/webapp/metrics/actions/index.ts b/tensorboard/webapp/metrics/actions/index.ts index 3cd3ebc823..ff7ed0208a 100644 --- a/tensorboard/webapp/metrics/actions/index.ts +++ b/tensorboard/webapp/metrics/actions/index.ts @@ -27,6 +27,7 @@ import {CardState} from '../store/metrics_types'; import { CardId, HeaderEditInfo, + HeaderRemoveInfo, HeaderToggleInfo, HistogramMode, MinMaxStep, @@ -239,6 +240,11 @@ export const dataTableColumnEdited = createAction( props() ); +export const dataTableColumnRemoved = createAction( + '[Metrics] Data table column removed by delete button', + props() +); + export const dataTableColumnToggled = createAction( '[Metrics] Data table column toggled in edit menu', props() diff --git a/tensorboard/webapp/metrics/internal_types.ts b/tensorboard/webapp/metrics/internal_types.ts index 57ce7b8156..b5da51431f 100644 --- a/tensorboard/webapp/metrics/internal_types.ts +++ b/tensorboard/webapp/metrics/internal_types.ts @@ -99,6 +99,12 @@ export interface HeaderEditInfo { headers: ColumnHeader[]; } +export interface HeaderRemoveInfo { + headerType: ColumnHeaderType; + cardId: CardId; + dataTableMode: DataTableMode; +} + export interface HeaderToggleInfo { dataTableMode: DataTableMode; headerType: ColumnHeaderType; diff --git a/tensorboard/webapp/metrics/store/metrics_reducers.ts b/tensorboard/webapp/metrics/store/metrics_reducers.ts index 05b84c9ae0..fd3baf45a0 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers.ts @@ -1372,6 +1372,46 @@ const reducer = createReducer( singleSelectionHeaders: enabledNewHeaders.concat(disabledNewHeaders), }; }), + on(actions.dataTableColumnRemoved, (state, {headerType, cardId, dataTableMode}) => { + const targetedHeaders = + dataTableMode === DataTableMode.RANGE + ? state.rangeSelectionHeaders + : state.singleSelectionHeaders; + + const currentToggledHeaderIndex = targetedHeaders.findIndex( + (element) => element.type === headerType + ); + + // If the header is being enabled it goes at the bottom of the currently + // enabled headers. If it is being disabled it goes to the top of the + // currently disabled headers. + let newToggledHeaderIndex = getEnabledCount(targetedHeaders); + if (targetedHeaders[currentToggledHeaderIndex].enabled) { + newToggledHeaderIndex--; + } + const newHeaders = moveHeader( + currentToggledHeaderIndex, + newToggledHeaderIndex, + targetedHeaders + ); + + newHeaders[newToggledHeaderIndex] = { + ...newHeaders[newToggledHeaderIndex], + enabled: !newHeaders[newToggledHeaderIndex].enabled, + }; + + if (dataTableMode === DataTableMode.RANGE) { + return { + ...state, + rangeSelectionHeaders: newHeaders, + }; + } + + return { + ...state, + singleSelectionHeaders: newHeaders, + }; + }), on(actions.dataTableColumnToggled, (state, {dataTableMode, headerType}) => { const targetedHeaders = dataTableMode === DataTableMode.RANGE diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html index b6574e4973..28b03e2cd0 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html @@ -202,6 +202,7 @@ [smoothingEnabled]="smoothingEnabled" (sortDataBy)="sortDataBy($event)" (editColumnHeaders)="editColumnHeaders.emit($event)" + (removeColumn)="removeColumn($event)" > diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index 7f827a6f82..4687e25e10 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -44,7 +44,7 @@ import { TooltipDatum, } from '../../../widgets/line_chart_v2/types'; import {CardState} from '../../store'; -import {HeaderEditInfo, TooltipSort, XAxisType} from '../../types'; +import {HeaderEditInfo, HeaderRemoveInfo, TooltipSort, XAxisType} from '../../types'; import { MinMaxStep, ScalarCardDataSeries, @@ -114,6 +114,7 @@ export class ScalarCardComponent { @Output() onDataTableSorting = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); @Output() openTableEditMenuToMode = new EventEmitter(); + @Output() onRemoveColumn = new EventEmitter(); @Output() onLineChartZoom = new EventEmitter(); @@ -145,6 +146,18 @@ export class ScalarCardComponent { this.onDataTableSorting.emit(sortingInfo); } + removeColumn(headerRemoveInfo: HeaderRemoveInfo) { + // this.sortingInfo = sortingInfo; + const removeInfo = { + dataTableMode: this.rangeEnabled + ? DataTableMode.RANGE + : DataTableMode.SINGLE, + headerType: headerRemoveInfo.headerType, + cardId: this.cardId + }; + this.onRemoveColumn.emit(removeInfo); + } + resetDomain() { if (this.lineChart) { this.lineChart.viewBoxReset(); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index 848865fd5f..a01c7a93c5 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -71,6 +71,7 @@ import {ScaleType} from '../../../widgets/line_chart_v2/types'; import { cardViewBoxChanged, dataTableColumnEdited, + dataTableColumnRemoved, metricsCardFullSizeToggled, metricsCardStateUpdated, sortingDataTable, @@ -92,7 +93,7 @@ import { getMetricsXAxisType, RunToSeries, } from '../../store'; -import {CardId, CardMetadata, HeaderEditInfo, XAxisType} from '../../types'; +import {CardId, CardMetadata, HeaderEditInfo, HeaderRemoveInfo, XAxisType} from '../../types'; import {getFilteredRenderableRunsIdsFromRoute} from '../main_view/common_selectors'; import {CardRenderer} from '../metrics_view_types'; import {getTagDisplayName} from '../utils'; @@ -188,6 +189,7 @@ function isMinMaxStepValid(minMax: MinMaxStep | undefined): boolean { (editColumnHeaders)="editColumnHeaders($event)" (onCardStateChanged)="onCardStateChanged($event)" (openTableEditMenuToMode)="openTableEditMenuToMode($event)" + (onRemoveColumn)="onRemoveColumn($event)" > `, styles: [ @@ -682,4 +684,8 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { openTableEditMenuToMode(tableMode: DataTableMode) { this.store.dispatch(metricsSlideoutMenuOpened({mode: tableMode})); } + + onRemoveColumn(headerRemoveInfo: HeaderRemoveInfo) { + this.store.dispatch(dataTableColumnRemoved(headerRemoveInfo)); + } } diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts index b4e5e46d07..aa54c29491 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts @@ -36,6 +36,8 @@ import { SortingOrder, } from '../../../widgets/data_table/types'; import {isDatumVisible} from './utils'; +// import { CardId, HeaderRemoveInfo } from '../../../metrics/internal_types'; +// import {CardId} from '../../types'; @Component({ selector: 'scalar-card-data-table', @@ -48,6 +50,7 @@ import {isDatumVisible} from './utils'; [smoothingEnabled]="smoothingEnabled" (sortDataBy)="sortDataBy.emit($event)" (orderColumns)="orderColumns($event)" + (removeColumn)="removeColumn.emit($event)" > `, changeDetection: ChangeDetectionStrategy.OnPush, @@ -60,9 +63,14 @@ export class ScalarCardDataTable { @Input() sortingInfo!: SortingInfo; @Input() columnCustomizationEnabled!: boolean; @Input() smoothingEnabled!: boolean; + // @Input() dataTableMode!: DataTableMode; + // @Input() cardId!: CardId; @Output() sortDataBy = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); + @Output() removeColumn = new EventEmitter<{ + headerType: ColumnHeaderType; + }>(); getMinPointInRange( points: ScalarCardPoint[], diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index 9f3eaa6f93..fd4484629e 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -30,7 +30,13 @@ [ngClass]="getHeaderHighlightStyle(header.name)" > - +
+ +
(); @Output() orderColumns = new EventEmitter(); + @Output() removeColumn = new EventEmitter<{ + headerType: ColumnHeaderType; + }>(); readonly ColumnHeaders = ColumnHeaderType; readonly SortingOrder = SortingOrder; @@ -201,4 +204,15 @@ export class DataTableComponent implements OnDestroy { return name === element.name; }); } + + clickRemoveColumn(header: ColumnHeader) { + // const headerRemoveInfo: HeaderRemoveInfo = { + // dataTableMode: this.dataTableMode, + // headerType: header.type, + // cardId: this.cardId + // }; + this.removeColumn.emit({ + headerType: header.type, + }) + } } From e17b2b1f4a985862ee1dbd335c733a29d8f92199 Mon Sep 17 00:00:00 2001 From: Brenda Huang Date: Tue, 30 May 2023 20:40:51 +0000 Subject: [PATCH 02/12] omit data table mode awareness --- tensorboard/webapp/metrics/internal_types.ts | 1 - .../webapp/metrics/store/metrics_reducers.ts | 17 +++++++---------- .../store/metrics_store_internal_utils.ts | 19 +++++++++++++++++++ .../card_renderer/scalar_card_component.ts | 4 ---- .../card_renderer/scalar_card_data_table.ts | 2 -- .../data_table/data_table_component.ng.html | 16 +++++++++------- .../data_table/data_table_component.scss | 18 ++++++++++++++++++ .../data_table/data_table_component.ts | 5 ----- 8 files changed, 53 insertions(+), 29 deletions(-) diff --git a/tensorboard/webapp/metrics/internal_types.ts b/tensorboard/webapp/metrics/internal_types.ts index b5da51431f..83d8ec8d1b 100644 --- a/tensorboard/webapp/metrics/internal_types.ts +++ b/tensorboard/webapp/metrics/internal_types.ts @@ -102,7 +102,6 @@ export interface HeaderEditInfo { export interface HeaderRemoveInfo { headerType: ColumnHeaderType; cardId: CardId; - dataTableMode: DataTableMode; } export interface HeaderToggleInfo { diff --git a/tensorboard/webapp/metrics/store/metrics_reducers.ts b/tensorboard/webapp/metrics/store/metrics_reducers.ts index fd3baf45a0..eb5f7798a2 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers.ts @@ -62,6 +62,7 @@ import { generateNextPinnedCardMappings, generateScalarCardMinMaxStep, getCardId, + getCardRangeSelectionEnabled, getRunIds, getTimeSeriesLoadable, } from './metrics_store_internal_utils'; @@ -1372,19 +1373,15 @@ const reducer = createReducer( singleSelectionHeaders: enabledNewHeaders.concat(disabledNewHeaders), }; }), - on(actions.dataTableColumnRemoved, (state, {headerType, cardId, dataTableMode}) => { - const targetedHeaders = - dataTableMode === DataTableMode.RANGE + on(actions.dataTableColumnRemoved, (state, {headerType, cardId}) => { + const rangeSelectionEnabled = getCardRangeSelectionEnabled(state, cardId); + const targetedHeaders = rangeSelectionEnabled ? state.rangeSelectionHeaders : state.singleSelectionHeaders; - const currentToggledHeaderIndex = targetedHeaders.findIndex( (element) => element.type === headerType ); - // If the header is being enabled it goes at the bottom of the currently - // enabled headers. If it is being disabled it goes to the top of the - // currently disabled headers. let newToggledHeaderIndex = getEnabledCount(targetedHeaders); if (targetedHeaders[currentToggledHeaderIndex].enabled) { newToggledHeaderIndex--; @@ -1400,16 +1397,16 @@ const reducer = createReducer( enabled: !newHeaders[newToggledHeaderIndex].enabled, }; - if (dataTableMode === DataTableMode.RANGE) { + if (rangeSelectionEnabled) { return { ...state, - rangeSelectionHeaders: newHeaders, + rangeSelectionHeaders: newHeaders }; } return { ...state, - singleSelectionHeaders: newHeaders, + singleSelectionHeaders: newHeaders }; }), on(actions.dataTableColumnToggled, (state, {dataTableMode, headerType}) => { diff --git a/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts b/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts index efd57e56bc..78e63fd935 100644 --- a/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts +++ b/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts @@ -601,6 +601,25 @@ export function getCardSelectionStateToBoolean( } } +export function getCardRangeSelectionEnabled( + state: MetricsState, + cardId: CardId +) { + const cardStateMap = state.cardStateMap; + const globalRangeSelectionEnabled = state.rangeSelectionEnabled; + const linkedTimeEnabled = state.linkedTimeEnabled; + + if (linkedTimeEnabled) { + return globalRangeSelectionEnabled; + } + + const cardState = cardStateMap[cardId]; + return getCardSelectionStateToBoolean( + cardState?.rangeSelectionOverride, + globalRangeSelectionEnabled + ); +} + export const TEST_ONLY = { getImageCardSteps, getSelectedSteps, diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index 4687e25e10..5a677f51ce 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -147,11 +147,7 @@ export class ScalarCardComponent { } removeColumn(headerRemoveInfo: HeaderRemoveInfo) { - // this.sortingInfo = sortingInfo; const removeInfo = { - dataTableMode: this.rangeEnabled - ? DataTableMode.RANGE - : DataTableMode.SINGLE, headerType: headerRemoveInfo.headerType, cardId: this.cardId }; diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts index aa54c29491..faaabcacc6 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts @@ -36,8 +36,6 @@ import { SortingOrder, } from '../../../widgets/data_table/types'; import {isDatumVisible} from './utils'; -// import { CardId, HeaderRemoveInfo } from '../../../metrics/internal_types'; -// import {CardId} from '../../types'; @Component({ selector: 'scalar-card-data-table', diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index fd4484629e..e4413971fe 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -30,13 +30,6 @@ [ngClass]="getHeaderHighlightStyle(header.name)" > -
- -
+
+ + +
diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.scss b/tensorboard/webapp/widgets/data_table/data_table_component.scss index 79b97a2b30..911ae54739 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.scss +++ b/tensorboard/webapp/widgets/data_table/data_table_component.scss @@ -93,6 +93,24 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); border-radius: 5px; } + .delete-icon-container { + width: 12px; + height: 12px; + border-radius: 5px; + top: 0; + } + + .delete-icon-container:hover { + background-color: mat.get-color-from-palette(mat.$gray-palette, 500); + color: mat.get-color-from-palette(mat.$gray-palette, 200); + cursor: pointer; + + @include tb-dark-theme { + background-color: mat.get-color-from-palette(mat.$gray-palette, 700); + color: mat.get-color-from-palette(mat.$gray-palette, 300); + } + } + .show { opacity: 1; } diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ts b/tensorboard/webapp/widgets/data_table/data_table_component.ts index c7fd683137..45fdd396ab 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ts @@ -206,11 +206,6 @@ export class DataTableComponent implements OnDestroy { } clickRemoveColumn(header: ColumnHeader) { - // const headerRemoveInfo: HeaderRemoveInfo = { - // dataTableMode: this.dataTableMode, - // headerType: header.type, - // cardId: this.cardId - // }; this.removeColumn.emit({ headerType: header.type, }) From ff883244da8901c1a0a0203ecc77761e65c1264c Mon Sep 17 00:00:00 2001 From: Brenda Huang Date: Wed, 31 May 2023 20:32:48 +0000 Subject: [PATCH 03/12] add button styling and use toggle functionality --- tensorboard/webapp/metrics/internal_types.ts | 3 +- .../webapp/metrics/store/metrics_reducers.ts | 102 +++++++----------- .../store/metrics_store_internal_utils.ts | 12 +-- .../card_renderer/scalar_card_component.ts | 19 ++-- .../card_renderer/scalar_card_container.ts | 14 ++- .../data_table/data_table_component.ng.html | 6 +- .../data_table/data_table_component.scss | 9 +- .../data_table/data_table_component.ts | 2 +- 8 files changed, 73 insertions(+), 94 deletions(-) diff --git a/tensorboard/webapp/metrics/internal_types.ts b/tensorboard/webapp/metrics/internal_types.ts index 83d8ec8d1b..825bf9e968 100644 --- a/tensorboard/webapp/metrics/internal_types.ts +++ b/tensorboard/webapp/metrics/internal_types.ts @@ -105,8 +105,9 @@ export interface HeaderRemoveInfo { } export interface HeaderToggleInfo { - dataTableMode: DataTableMode; + dataTableMode?: DataTableMode; headerType: ColumnHeaderType; + cardId?: CardId; } export const SCALARS_SMOOTHING_MIN = 0; diff --git a/tensorboard/webapp/metrics/store/metrics_reducers.ts b/tensorboard/webapp/metrics/store/metrics_reducers.ts index eb5f7798a2..b5f9bee0b3 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers.ts @@ -1373,82 +1373,56 @@ const reducer = createReducer( singleSelectionHeaders: enabledNewHeaders.concat(disabledNewHeaders), }; }), - on(actions.dataTableColumnRemoved, (state, {headerType, cardId}) => { - const rangeSelectionEnabled = getCardRangeSelectionEnabled(state, cardId); - const targetedHeaders = rangeSelectionEnabled - ? state.rangeSelectionHeaders - : state.singleSelectionHeaders; - const currentToggledHeaderIndex = targetedHeaders.findIndex( - (element) => element.type === headerType - ); - - let newToggledHeaderIndex = getEnabledCount(targetedHeaders); - if (targetedHeaders[currentToggledHeaderIndex].enabled) { - newToggledHeaderIndex--; - } - const newHeaders = moveHeader( - currentToggledHeaderIndex, - newToggledHeaderIndex, - targetedHeaders - ); - - newHeaders[newToggledHeaderIndex] = { - ...newHeaders[newToggledHeaderIndex], - enabled: !newHeaders[newToggledHeaderIndex].enabled, - }; + on( + actions.dataTableColumnToggled, + (state, {dataTableMode, headerType, cardId}) => { + let rangeSelectionEnabled; - if (rangeSelectionEnabled) { - return { - ...state, - rangeSelectionHeaders: newHeaders - }; - } + if (cardId) { + rangeSelectionEnabled = getCardRangeSelectionEnabled(state, cardId); + } else { + rangeSelectionEnabled = dataTableMode === DataTableMode.RANGE; + } - return { - ...state, - singleSelectionHeaders: newHeaders - }; - }), - on(actions.dataTableColumnToggled, (state, {dataTableMode, headerType}) => { - const targetedHeaders = - dataTableMode === DataTableMode.RANGE + const targetedHeaders = rangeSelectionEnabled ? state.rangeSelectionHeaders : state.singleSelectionHeaders; - const currentToggledHeaderIndex = targetedHeaders.findIndex( - (element) => element.type === headerType - ); + const currentToggledHeaderIndex = targetedHeaders.findIndex( + (element) => element.type === headerType + ); - // If the header is being enabled it goes at the bottom of the currently - // enabled headers. If it is being disabled it goes to the top of the - // currently disabled headers. - let newToggledHeaderIndex = getEnabledCount(targetedHeaders); - if (targetedHeaders[currentToggledHeaderIndex].enabled) { - newToggledHeaderIndex--; - } - const newHeaders = moveHeader( - currentToggledHeaderIndex, - newToggledHeaderIndex, - targetedHeaders - ); + // If the header is being enabled it goes at the bottom of the currently + // enabled headers. If it is being disabled it goes to the top of the + // currently disabled headers. + let newToggledHeaderIndex = getEnabledCount(targetedHeaders); + if (targetedHeaders[currentToggledHeaderIndex].enabled) { + newToggledHeaderIndex--; + } + const newHeaders = moveHeader( + currentToggledHeaderIndex, + newToggledHeaderIndex, + targetedHeaders + ); - newHeaders[newToggledHeaderIndex] = { - ...newHeaders[newToggledHeaderIndex], - enabled: !newHeaders[newToggledHeaderIndex].enabled, - }; + newHeaders[newToggledHeaderIndex] = { + ...newHeaders[newToggledHeaderIndex], + enabled: !newHeaders[newToggledHeaderIndex].enabled, + }; + + if (rangeSelectionEnabled) { + return { + ...state, + rangeSelectionHeaders: newHeaders, + }; + } - if (dataTableMode === DataTableMode.RANGE) { return { ...state, - rangeSelectionHeaders: newHeaders, + singleSelectionHeaders: newHeaders, }; } - - return { - ...state, - singleSelectionHeaders: newHeaders, - }; - }), + ), on(actions.metricsToggleVisiblePlugin, (state, {plugin}) => { let nextFilteredPluginTypes = new Set(state.filteredPluginTypes); if (nextFilteredPluginTypes.has(plugin)) { diff --git a/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts b/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts index 78e63fd935..b90ac84f96 100644 --- a/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts +++ b/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts @@ -602,8 +602,8 @@ export function getCardSelectionStateToBoolean( } export function getCardRangeSelectionEnabled( - state: MetricsState, - cardId: CardId + state: MetricsState, + cardId: CardId ) { const cardStateMap = state.cardStateMap; const globalRangeSelectionEnabled = state.rangeSelectionEnabled; @@ -614,10 +614,10 @@ export function getCardRangeSelectionEnabled( } const cardState = cardStateMap[cardId]; - return getCardSelectionStateToBoolean( - cardState?.rangeSelectionOverride, - globalRangeSelectionEnabled - ); + return getCardSelectionStateToBoolean( + cardState?.rangeSelectionOverride, + globalRangeSelectionEnabled + ); } export const TEST_ONLY = { diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index 5a677f51ce..d00388cacb 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -44,7 +44,12 @@ import { TooltipDatum, } from '../../../widgets/line_chart_v2/types'; import {CardState} from '../../store'; -import {HeaderEditInfo, HeaderRemoveInfo, TooltipSort, XAxisType} from '../../types'; +import { + HeaderEditInfo, + HeaderToggleInfo, + TooltipSort, + XAxisType, +} from '../../types'; import { MinMaxStep, ScalarCardDataSeries, @@ -114,7 +119,7 @@ export class ScalarCardComponent { @Output() onDataTableSorting = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); @Output() openTableEditMenuToMode = new EventEmitter(); - @Output() onRemoveColumn = new EventEmitter(); + @Output() onRemoveColumn = new EventEmitter(); @Output() onLineChartZoom = new EventEmitter(); @@ -146,12 +151,12 @@ export class ScalarCardComponent { this.onDataTableSorting.emit(sortingInfo); } - removeColumn(headerRemoveInfo: HeaderRemoveInfo) { - const removeInfo = { - headerType: headerRemoveInfo.headerType, - cardId: this.cardId + removeColumn(headerToggleInfo: HeaderToggleInfo) { + const toggleInfo = { + headerType: headerToggleInfo.headerType, + cardId: this.cardId, }; - this.onRemoveColumn.emit(removeInfo); + this.onRemoveColumn.emit(toggleInfo); } resetDomain() { diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index a01c7a93c5..c91917f9b0 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -71,7 +71,7 @@ import {ScaleType} from '../../../widgets/line_chart_v2/types'; import { cardViewBoxChanged, dataTableColumnEdited, - dataTableColumnRemoved, + dataTableColumnToggled, metricsCardFullSizeToggled, metricsCardStateUpdated, sortingDataTable, @@ -93,7 +93,13 @@ import { getMetricsXAxisType, RunToSeries, } from '../../store'; -import {CardId, CardMetadata, HeaderEditInfo, HeaderRemoveInfo, XAxisType} from '../../types'; +import { + CardId, + CardMetadata, + HeaderEditInfo, + HeaderToggleInfo, + XAxisType, +} from '../../types'; import {getFilteredRenderableRunsIdsFromRoute} from '../main_view/common_selectors'; import {CardRenderer} from '../metrics_view_types'; import {getTagDisplayName} from '../utils'; @@ -685,7 +691,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { this.store.dispatch(metricsSlideoutMenuOpened({mode: tableMode})); } - onRemoveColumn(headerRemoveInfo: HeaderRemoveInfo) { - this.store.dispatch(dataTableColumnRemoved(headerRemoveInfo)); + onRemoveColumn(headerToggleInfo: HeaderToggleInfo) { + this.store.dispatch(dataTableColumnToggled(headerToggleInfo)); } } diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index e4413971fe..8b0405a1b3 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -46,10 +46,10 @@ - + > + diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.scss b/tensorboard/webapp/widgets/data_table/data_table_component.scss index 911ae54739..50e29f155e 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.scss +++ b/tensorboard/webapp/widgets/data_table/data_table_component.scss @@ -97,18 +97,11 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); width: 12px; height: 12px; border-radius: 5px; - top: 0; } .delete-icon-container:hover { - background-color: mat.get-color-from-palette(mat.$gray-palette, 500); - color: mat.get-color-from-palette(mat.$gray-palette, 200); + background-color: mat.get-color-from-palette(mat.$gray-palette, 300); cursor: pointer; - - @include tb-dark-theme { - background-color: mat.get-color-from-palette(mat.$gray-palette, 700); - color: mat.get-color-from-palette(mat.$gray-palette, 300); - } } .show { diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ts b/tensorboard/webapp/widgets/data_table/data_table_component.ts index 45fdd396ab..239ce3c0b8 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ts @@ -208,6 +208,6 @@ export class DataTableComponent implements OnDestroy { clickRemoveColumn(header: ColumnHeader) { this.removeColumn.emit({ headerType: header.type, - }) + }); } } From 3380dc8a8c2d1edf4671ebb6173287cd1f750b3b Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Fri, 2 Jun 2023 18:37:28 +0000 Subject: [PATCH 04/12] feature flag and unit testing --- .../metrics/store/metrics_reducers_test.ts | 357 +++++++++++++++++- .../metrics_store_internal_utils_test.ts | 88 +++++ .../webapp/metrics/views/card_renderer/BUILD | 1 + .../card_renderer/scalar_card_data_table.ts | 2 - .../views/card_renderer/scalar_card_test.ts | 39 ++ tensorboard/webapp/widgets/data_table/BUILD | 9 + .../data_table/data_table_component.ng.html | 3 +- .../data_table/data_table_component.ts | 16 +- .../widgets/data_table/data_table_test.ts | 135 +++++++ 9 files changed, 643 insertions(+), 7 deletions(-) diff --git a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts index cd81c60377..d92da4a9a4 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts @@ -1964,7 +1964,7 @@ describe('metrics reducers', () => { }); describe('dataTableColumnToggled', () => { - it('moves header down to the disabled headers when toggling to disabled', () => { + it('moves header down to the disabled headers when toggling to disabled with data table mode input', () => { const beforeState = buildMetricsState({ rangeSelectionHeaders: [ { @@ -2042,7 +2042,7 @@ describe('metrics reducers', () => { ]); }); - it('moves header up to the enabled headers when toggling to enabled', () => { + it('moves header up to the enabled headers when toggling to enabled with data table mode input', () => { const beforeState = buildMetricsState({ rangeSelectionHeaders: [ { @@ -2379,6 +2379,359 @@ describe('metrics reducers', () => { }, ]); }); + + it('moves header down to the disabled headers when column is removed with card id input', () => { + const beforeState = buildMetricsState({ + rangeSelectionHeaders: [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.START_VALUE, + name: 'startValue', + displayName: 'Start Value', + enabled: true, + }, + { + type: ColumnHeaderType.END_VALUE, + name: 'endValue', + displayName: 'End Value', + enabled: true, + }, + { + type: ColumnHeaderType.MIN_VALUE, + name: 'minValue', + displayName: 'Min', + enabled: false, + }, + { + type: ColumnHeaderType.MAX_VALUE, + name: 'maxValue', + displayName: 'Max', + enabled: false, + }, + ], + cardStateMap: { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }, + }); + + const nextState = reducers( + beforeState, + actions.dataTableColumnToggled({ + cardId: 'card1', + headerType: ColumnHeaderType.RUN, + }) + ); + + expect(nextState.rangeSelectionHeaders).toEqual([ + { + type: ColumnHeaderType.START_VALUE, + name: 'startValue', + displayName: 'Start Value', + enabled: true, + }, + { + type: ColumnHeaderType.END_VALUE, + name: 'endValue', + displayName: 'End Value', + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: false, + }, + { + type: ColumnHeaderType.MIN_VALUE, + name: 'minValue', + displayName: 'Min', + enabled: false, + }, + { + type: ColumnHeaderType.MAX_VALUE, + name: 'maxValue', + displayName: 'Max', + enabled: false, + }, + ]); + }); + + it('only changes range selection headers when given card has rangeSelectionOverride ENABLED', () => { + const beforeState = buildMetricsState({ + rangeSelectionHeaders: [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.START_VALUE, + name: 'startValue', + displayName: 'Start Value', + enabled: true, + }, + { + type: ColumnHeaderType.END_VALUE, + name: 'endValue', + displayName: 'End Value', + enabled: true, + }, + { + type: ColumnHeaderType.MIN_VALUE, + name: 'minValue', + displayName: 'Min', + enabled: false, + }, + { + type: ColumnHeaderType.MAX_VALUE, + name: 'maxValue', + displayName: 'Max', + enabled: false, + }, + ], + singleSelectionHeaders: [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.STEP, + name: 'step', + displayName: 'Step', + enabled: true, + }, + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.RELATIVE_TIME, + name: 'relativeTime', + displayName: 'Relative', + enabled: false, + }, + ], + cardStateMap: { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }, + }); + + const nextState = reducers( + beforeState, + actions.dataTableColumnToggled({ + cardId: 'card1', + headerType: ColumnHeaderType.MAX_VALUE, + }) + ); + + expect(nextState.rangeSelectionHeaders).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.START_VALUE, + name: 'startValue', + displayName: 'Start Value', + enabled: true, + }, + { + type: ColumnHeaderType.END_VALUE, + name: 'endValue', + displayName: 'End Value', + enabled: true, + }, + { + type: ColumnHeaderType.MAX_VALUE, + name: 'maxValue', + displayName: 'Max', + enabled: true, + }, + { + type: ColumnHeaderType.MIN_VALUE, + name: 'minValue', + displayName: 'Min', + enabled: false, + }, + ]); + expect(nextState.singleSelectionHeaders).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.STEP, + name: 'step', + displayName: 'Step', + enabled: true, + }, + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.RELATIVE_TIME, + name: 'relativeTime', + displayName: 'Relative', + enabled: false, + }, + ]); + }); + + it('only changes single selection headers when given card has rangeSelectionOverride DISABLED', () => { + const beforeState = buildMetricsState({ + rangeSelectionHeaders: [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.START_VALUE, + name: 'startValue', + displayName: 'Start Value', + enabled: true, + }, + { + type: ColumnHeaderType.END_VALUE, + name: 'endValue', + displayName: 'End Value', + enabled: true, + }, + { + type: ColumnHeaderType.MIN_VALUE, + name: 'minValue', + displayName: 'Min', + enabled: false, + }, + { + type: ColumnHeaderType.MAX_VALUE, + name: 'maxValue', + displayName: 'Max', + enabled: false, + }, + ], + singleSelectionHeaders: [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.STEP, + name: 'step', + displayName: 'Step', + enabled: true, + }, + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.RELATIVE_TIME, + name: 'relativeTime', + displayName: 'Relative', + enabled: false, + }, + ], + cardStateMap: { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + }, + }); + + const nextState = reducers( + beforeState, + actions.dataTableColumnToggled({ + cardId: 'card1', + headerType: ColumnHeaderType.STEP, + }) + ); + + expect(nextState.rangeSelectionHeaders).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.START_VALUE, + name: 'startValue', + displayName: 'Start Value', + enabled: true, + }, + { + type: ColumnHeaderType.END_VALUE, + name: 'endValue', + displayName: 'End Value', + enabled: true, + }, + { + type: ColumnHeaderType.MIN_VALUE, + name: 'minValue', + displayName: 'Min', + enabled: false, + }, + { + type: ColumnHeaderType.MAX_VALUE, + name: 'maxValue', + displayName: 'Max', + enabled: false, + }, + ]); + expect(nextState.singleSelectionHeaders).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.STEP, + name: 'step', + displayName: 'Step', + enabled: false, + }, + { + type: ColumnHeaderType.RELATIVE_TIME, + name: 'relativeTime', + displayName: 'Relative', + enabled: false, + }, + ]); + }); }); }); diff --git a/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts b/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts index 382e02f02c..0970f6e179 100644 --- a/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts @@ -32,6 +32,7 @@ import { generateScalarCardMinMaxStep, getCardId, getCardSelectionStateToBoolean, + getCardRangeSelectionEnabled, getMinMaxStepFromCardState, getPinnedCardId, getRunIds, @@ -1278,4 +1279,91 @@ describe('metrics store utils', () => { expect(getCardSelectionStateToBoolean(undefined, false)).toBeFalse(); }); }); + + describe('getCardRangeSelectionEnabled', () => { + it('returns card specific value when defined', () => { + expect( + getCardRangeSelectionEnabled( + buildMetricsState({ + rangeSelectionEnabled: false, + cardStateMap: { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }, + }), + 'card1' + ) + ).toBeTrue(); + expect( + getCardRangeSelectionEnabled( + buildMetricsState({ + rangeSelectionEnabled: true, + cardStateMap: { + card1: { + rangeSelectionOverride: + CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + }, + }), + 'card1' + ) + ).toBeFalse(); + }); + + it('returns global value when card specific value is not defined', () => { + expect( + getCardRangeSelectionEnabled( + buildMetricsState({ + rangeSelectionEnabled: true, + cardStateMap: { + card1: {}, + }, + }), + 'card1' + ) + ).toBeTrue(); + expect( + getCardRangeSelectionEnabled( + buildMetricsState({ + rangeSelectionEnabled: false, + }), + 'card1' + ) + ).toBeFalse(); + }); + + it('returns global value when linked time is enabled', () => { + expect( + getCardRangeSelectionEnabled( + buildMetricsState({ + rangeSelectionEnabled: true, + linkedTimeEnabled: true, + cardStateMap: { + card1: { + rangeSelectionOverride: + CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + }, + }), + 'card1' + ) + ).toBeTrue(); + + expect( + getCardRangeSelectionEnabled( + buildMetricsState({ + rangeSelectionEnabled: false, + linkedTimeEnabled: true, + cardStateMap: { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }, + }), + 'card1' + ) + ).toBeFalse(); + }); + }); }); diff --git a/tensorboard/webapp/metrics/views/card_renderer/BUILD b/tensorboard/webapp/metrics/views/card_renderer/BUILD index d86a65d8e7..2a5debd900 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/BUILD +++ b/tensorboard/webapp/metrics/views/card_renderer/BUILD @@ -382,6 +382,7 @@ tf_ts_library( "//tensorboard/webapp/metrics/actions", "//tensorboard/webapp/metrics/data_source", "//tensorboard/webapp/metrics/store", + "//tensorboard/webapp/metrics/store:types", "//tensorboard/webapp/metrics/views/main_view:common_selectors", "//tensorboard/webapp/runs/store:testing", "//tensorboard/webapp/runs/store:types", diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts index faaabcacc6..11df7428de 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts @@ -61,8 +61,6 @@ export class ScalarCardDataTable { @Input() sortingInfo!: SortingInfo; @Input() columnCustomizationEnabled!: boolean; @Input() smoothingEnabled!: boolean; - // @Input() dataTableMode!: DataTableMode; - // @Input() cardId!: CardId; @Output() sortDataBy = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index c133e43cdc..1bfc3dbc3c 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -82,6 +82,7 @@ import { timeSelectionChanged, metricsSlideoutMenuOpened, dataTableColumnEdited, + dataTableColumnToggled, } from '../../actions'; import {PluginType} from '../../data_source'; import { @@ -122,6 +123,7 @@ import {VisLinkedTimeSelectionWarningModule} from './vis_linked_time_selection_w import {Extent} from '../../../widgets/line_chart_v2/lib/public_types'; import {provideMockTbStore} from '../../../testing/utils'; import * as commonSelectors from '../main_view/common_selectors'; +import {CardFeatureOverride} from '../../store/metrics_types'; @Component({ selector: 'line-chart', @@ -4305,6 +4307,43 @@ describe('scalar card', () => { }), ]); })); + + it('emits dataTableColumnToggled when onRemoveColumn is called', fakeAsync(() => { + store.overrideSelector(getSingleSelectionHeaders, [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: false, + }, + ]); + store.overrideSelector(getCardStateMap, { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }); + const fixture = createComponent('card1'); + fixture.detectChanges(); + + const headerToggleInfo = { + cardId: 'card1', + headerType: ColumnHeaderType.RUN, + }; + fixture.componentInstance.onRemoveColumn(headerToggleInfo); + + expect(dispatchedActions).toEqual([ + dataTableColumnToggled({ + cardId: 'card1', + headerType: ColumnHeaderType.RUN, + }), + ]); + })); }); }); diff --git a/tensorboard/webapp/widgets/data_table/BUILD b/tensorboard/webapp/widgets/data_table/BUILD index 68b2b9607c..5f4c43788e 100644 --- a/tensorboard/webapp/widgets/data_table/BUILD +++ b/tensorboard/webapp/widgets/data_table/BUILD @@ -35,11 +35,15 @@ tf_ng_module( deps = [ ":data_table_header", ":types", + "//tensorboard/webapp:app_state", "//tensorboard/webapp/angular:expect_angular_material_icon", + "//tensorboard/webapp/feature_flag/store", "//tensorboard/webapp/metrics/views/card_renderer:scalar_card_types", "//tensorboard/webapp/widgets/line_chart_v2/lib:formatter", "@npm//@angular/common", "@npm//@angular/core", + "@npm//@ngrx/store", + "@npm//rxjs", ], ) @@ -77,10 +81,15 @@ tf_ts_library( deps = [ ":data_table", ":types", + "//tensorboard/webapp:app_state", "//tensorboard/webapp/angular:expect_angular_core_testing", "//tensorboard/webapp/angular:expect_angular_material_icon", + "//tensorboard/webapp/feature_flag/store", + "//tensorboard/webapp/testing:utils", "@npm//@angular/core", "@npm//@angular/platform-browser", + "@npm//@ngrx/store", "@npm//@types/jasmine", + "@npm//rxjs", ], ) diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index 8b0405a1b3..07176dd966 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -42,9 +42,8 @@ svgIcon="arrow_downward_24px" > -
+
(false); + + constructor(private readonly store: Store) {} ngOnDestroy() { document.removeEventListener('dragover', preventDefault); } + ngOnInit() { + this.store.select(getEnableHparamsInTimeSeries).subscribe((enabled) => { + this.HParamsEnabled.next(enabled); + }); + } + getFormattedDataForColumn( columnHeader: ColumnHeaderType, datum: string | number | undefined diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts index 354e208efd..4ec6bb0ca4 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_test.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts @@ -26,6 +26,11 @@ import { } from './types'; import {DataTableComponent} from './data_table_component'; import {DataTableModule} from './data_table_module'; +import {getEnableHparamsInTimeSeries} from '../../feature_flag/store/feature_flag_selectors'; +import {MockStore} from '@ngrx/store/testing'; +import {State} from '../../app_state'; +import {Store} from '@ngrx/store'; +import {provideMockTbStore} from '../../testing/utils'; @Component({ selector: 'testable-comp', @@ -38,6 +43,7 @@ import {DataTableModule} from './data_table_module'; [smoothingEnabled]="smoothingEnabled" (sortDataBy)="sortDataBy($event)" (orderColumns)="orderColumns($event)" + (removeColumn)="removeColumn($event)" > `, }) @@ -52,16 +58,21 @@ class TestableComponent { @Input() sortDataBy!: (sortingInfo: SortingInfo) => void; @Input() orderColumns!: (newOrder: ColumnHeaderType[]) => void; + @Input() removeColumn!: (headerType: ColumnHeaderType) => void; } describe('data table', () => { + let store: MockStore; let sortDataBySpy: jasmine.Spy; let orderColumnsSpy: jasmine.Spy; + let removeColumnSpy: jasmine.Spy; beforeEach(async () => { await TestBed.configureTestingModule({ declarations: [TestableComponent, DataTableComponent], imports: [MatIconModule, DataTableModule], + providers: [provideMockTbStore()], }).compileComponents(); + store = TestBed.inject>(Store) as MockStore; }); function createComponent(input: { @@ -88,6 +99,9 @@ describe('data table', () => { orderColumnsSpy = jasmine.createSpy(); fixture.componentInstance.orderColumns = orderColumnsSpy; + removeColumnSpy = jasmine.createSpy(); + fixture.componentInstance.removeColumn = removeColumnSpy; + return fixture; } @@ -789,4 +803,125 @@ describe('data table', () => { expect(dataElements[3].nativeElement.innerText).toBe('1'); expect(dataElements.length).toBe(4); }); + + describe('delete column button', () => { + it('emits removeColumn event when delete button clicked', () => { + store.overrideSelector(getEnableHparamsInTimeSeries, true); + const fixture = createComponent({ + headers: [ + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.STEP, + name: 'step', + displayName: 'Step', + enabled: true, + }, + { + type: ColumnHeaderType.RELATIVE_TIME, + name: 'relativeTime', + displayName: 'Relative', + enabled: true, + }, + ], + }); + fixture.detectChanges(); + const headerElements = fixture.debugElement.queryAll( + By.css('.header > .col') + ); + + headerElements[3] + .queryAll(By.css('mat-icon'))[1] + .triggerEventHandler('click', {}); + expect(removeColumnSpy).toHaveBeenCalledOnceWith({ + headerType: ColumnHeaderType.STEP, + }); + }); + + it('renders delete button when hparam flag is on', () => { + store.overrideSelector(getEnableHparamsInTimeSeries, true); + const fixture = createComponent({ + headers: [ + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.STEP, + name: 'step', + displayName: 'Step', + enabled: true, + }, + { + type: ColumnHeaderType.RELATIVE_TIME, + name: 'relativeTime', + displayName: 'Relative', + enabled: true, + }, + ], + }); + fixture.detectChanges(); + const headerElements = fixture.debugElement.queryAll( + By.css('.header > .col') + ); + + expect(headerElements[3].queryAll(By.css('mat-icon'))[1]).toBeTruthy(); + }); + + it('does not render delete button when hparam flag is off', () => { + store.overrideSelector(getEnableHparamsInTimeSeries, false); + const fixture = createComponent({ + headers: [ + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.STEP, + name: 'step', + displayName: 'Step', + enabled: true, + }, + { + type: ColumnHeaderType.RELATIVE_TIME, + name: 'relativeTime', + displayName: 'Relative', + enabled: true, + }, + ], + }); + fixture.detectChanges(); + const headerElements = fixture.debugElement.queryAll( + By.css('.header > .col') + ); + + expect(headerElements[3].queryAll(By.css('mat-icon'))[1]).toBeFalsy(); + }); + }); }); From 56c75d51d10fa8600b0e5629b9a78707b1e53f0f Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Fri, 2 Jun 2023 21:04:04 +0000 Subject: [PATCH 05/12] clean up unused code --- tensorboard/webapp/metrics/actions/index.ts | 8 +------- tensorboard/webapp/metrics/internal_types.ts | 5 ----- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/tensorboard/webapp/metrics/actions/index.ts b/tensorboard/webapp/metrics/actions/index.ts index ff7ed0208a..c12221344e 100644 --- a/tensorboard/webapp/metrics/actions/index.ts +++ b/tensorboard/webapp/metrics/actions/index.ts @@ -27,7 +27,6 @@ import {CardState} from '../store/metrics_types'; import { CardId, HeaderEditInfo, - HeaderRemoveInfo, HeaderToggleInfo, HistogramMode, MinMaxStep, @@ -240,13 +239,8 @@ export const dataTableColumnEdited = createAction( props() ); -export const dataTableColumnRemoved = createAction( - '[Metrics] Data table column removed by delete button', - props() -); - export const dataTableColumnToggled = createAction( - '[Metrics] Data table column toggled in edit menu', + '[Metrics] Data table column toggled in edit menu or delete button', props() ); diff --git a/tensorboard/webapp/metrics/internal_types.ts b/tensorboard/webapp/metrics/internal_types.ts index 825bf9e968..144cf83520 100644 --- a/tensorboard/webapp/metrics/internal_types.ts +++ b/tensorboard/webapp/metrics/internal_types.ts @@ -99,11 +99,6 @@ export interface HeaderEditInfo { headers: ColumnHeader[]; } -export interface HeaderRemoveInfo { - headerType: ColumnHeaderType; - cardId: CardId; -} - export interface HeaderToggleInfo { dataTableMode?: DataTableMode; headerType: ColumnHeaderType; From 2db5cd7de36cf051fb2211e5f1cc0e1dbe89213c Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Fri, 2 Jun 2023 23:17:57 +0000 Subject: [PATCH 06/12] single/range selection redundant logic --- .../webapp/metrics/store/metrics_reducers.ts | 12 +- .../webapp/metrics/store/metrics_selectors.ts | 14 +- .../store/metrics_store_internal_utils.ts | 10 +- .../metrics_store_internal_utils_test.ts | 129 ++++++++++-------- 4 files changed, 90 insertions(+), 75 deletions(-) diff --git a/tensorboard/webapp/metrics/store/metrics_reducers.ts b/tensorboard/webapp/metrics/store/metrics_reducers.ts index b5f9bee0b3..759968d835 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers.ts @@ -55,6 +55,7 @@ import { buildOrReturnStateWithPinnedCopy, buildOrReturnStateWithUnresolvedImportedPins, canCreateNewPins, + cardRangeSelectionEnabled, createPluginDataWithLoadable, createRunToLoadState, generateNextCardStepIndex, @@ -62,7 +63,6 @@ import { generateNextPinnedCardMappings, generateScalarCardMinMaxStep, getCardId, - getCardRangeSelectionEnabled, getRunIds, getTimeSeriesLoadable, } from './metrics_store_internal_utils'; @@ -1379,7 +1379,15 @@ const reducer = createReducer( let rangeSelectionEnabled; if (cardId) { - rangeSelectionEnabled = getCardRangeSelectionEnabled(state, cardId); + const cardStateMap = state.cardStateMap; + const globalRangeSelectionEnabled = state.rangeSelectionEnabled; + const linkedTimeEnabled = state.linkedTimeEnabled; + rangeSelectionEnabled = cardRangeSelectionEnabled( + cardStateMap, + globalRangeSelectionEnabled, + linkedTimeEnabled, + cardId + ); } else { rangeSelectionEnabled = dataTableMode === DataTableMode.RANGE; } diff --git a/tensorboard/webapp/metrics/store/metrics_selectors.ts b/tensorboard/webapp/metrics/store/metrics_selectors.ts index ffb705aa76..ff6ee2ab2d 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors.ts @@ -33,6 +33,7 @@ import {MinMaxStep} from '../views/card_renderer/scalar_card_types'; import {formatTimeSelection} from '../views/card_renderer/utils'; import * as storeUtils from './metrics_store_internal_utils'; import { + cardRangeSelectionEnabled, getCardSelectionStateToBoolean, getMinMaxStepFromCardState, } from './metrics_store_internal_utils'; @@ -485,14 +486,11 @@ export const getMetricsCardRangeSelectionEnabled = createSelector( cardStateMap: CardStateMap, cardId: CardId ) => { - if (linkedTimeEnabled) { - return globalRangeSelectionEnabled; - } - - const cardState = cardStateMap[cardId]; - return getCardSelectionStateToBoolean( - cardState?.rangeSelectionOverride, - globalRangeSelectionEnabled + return cardRangeSelectionEnabled( + cardStateMap, + globalRangeSelectionEnabled, + linkedTimeEnabled, + cardId ); } ); diff --git a/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts b/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts index b90ac84f96..b0e0932129 100644 --- a/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts +++ b/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts @@ -601,14 +601,12 @@ export function getCardSelectionStateToBoolean( } } -export function getCardRangeSelectionEnabled( - state: MetricsState, +export function cardRangeSelectionEnabled( + cardStateMap: CardStateMap, + globalRangeSelectionEnabled: boolean, + linkedTimeEnabled: boolean, cardId: CardId ) { - const cardStateMap = state.cardStateMap; - const globalRangeSelectionEnabled = state.rangeSelectionEnabled; - const linkedTimeEnabled = state.linkedTimeEnabled; - if (linkedTimeEnabled) { return globalRangeSelectionEnabled; } diff --git a/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts b/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts index 0970f6e179..d81b2bbd4a 100644 --- a/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts @@ -25,6 +25,7 @@ import { buildOrReturnStateWithPinnedCopy, buildOrReturnStateWithUnresolvedImportedPins, canCreateNewPins, + cardRangeSelectionEnabled, createPluginDataWithLoadable, createRunToLoadState, generateNextCardStepIndex, @@ -32,7 +33,6 @@ import { generateScalarCardMinMaxStep, getCardId, getCardSelectionStateToBoolean, - getCardRangeSelectionEnabled, getMinMaxStepFromCardState, getPinnedCardId, getRunIds, @@ -1280,88 +1280,99 @@ describe('metrics store utils', () => { }); }); - describe('getCardRangeSelectionEnabled', () => { + describe('cardRangeSelectionEnabled', () => { it('returns card specific value when defined', () => { + let cardStateMap = { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }; + let globalRangeSelectionEnabled = false; + let linkedTimeEnabled = false; + let cardId = 'card1'; + expect( - getCardRangeSelectionEnabled( - buildMetricsState({ - rangeSelectionEnabled: false, - cardStateMap: { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, - }, - }, - }), - 'card1' + cardRangeSelectionEnabled( + cardStateMap, + globalRangeSelectionEnabled, + linkedTimeEnabled, + cardId ) ).toBeTrue(); + cardStateMap = { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + }; + globalRangeSelectionEnabled = true; expect( - getCardRangeSelectionEnabled( - buildMetricsState({ - rangeSelectionEnabled: true, - cardStateMap: { - card1: { - rangeSelectionOverride: - CardFeatureOverride.OVERRIDE_AS_DISABLED, - }, - }, - }), - 'card1' + cardRangeSelectionEnabled( + cardStateMap, + globalRangeSelectionEnabled, + linkedTimeEnabled, + cardId ) ).toBeFalse(); }); it('returns global value when card specific value is not defined', () => { + let cardStateMap = { + card1: {}, + }; + let globalRangeSelectionEnabled = true; + let linkedTimeEnabled = false; + let cardId = 'card1'; expect( - getCardRangeSelectionEnabled( - buildMetricsState({ - rangeSelectionEnabled: true, - cardStateMap: { - card1: {}, - }, - }), - 'card1' + cardRangeSelectionEnabled( + cardStateMap, + globalRangeSelectionEnabled, + linkedTimeEnabled, + cardId ) ).toBeTrue(); + + globalRangeSelectionEnabled = false; + expect( - getCardRangeSelectionEnabled( - buildMetricsState({ - rangeSelectionEnabled: false, - }), - 'card1' + cardRangeSelectionEnabled( + cardStateMap, + globalRangeSelectionEnabled, + linkedTimeEnabled, + cardId ) ).toBeFalse(); }); it('returns global value when linked time is enabled', () => { + let cardStateMap = { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + }; + let globalRangeSelectionEnabled = true; + let linkedTimeEnabled = true; + let cardId = 'card1'; expect( - getCardRangeSelectionEnabled( - buildMetricsState({ - rangeSelectionEnabled: true, - linkedTimeEnabled: true, - cardStateMap: { - card1: { - rangeSelectionOverride: - CardFeatureOverride.OVERRIDE_AS_DISABLED, - }, - }, - }), - 'card1' + cardRangeSelectionEnabled( + cardStateMap, + globalRangeSelectionEnabled, + linkedTimeEnabled, + cardId ) ).toBeTrue(); + cardStateMap = { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }; + globalRangeSelectionEnabled = false; expect( - getCardRangeSelectionEnabled( - buildMetricsState({ - rangeSelectionEnabled: false, - linkedTimeEnabled: true, - cardStateMap: { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, - }, - }, - }), - 'card1' + cardRangeSelectionEnabled( + cardStateMap, + globalRangeSelectionEnabled, + linkedTimeEnabled, + cardId ) ).toBeFalse(); }); From bcf71d3e25258fd8152ca46f632f7e4521e5bb9b Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Tue, 6 Jun 2023 18:38:02 +0000 Subject: [PATCH 07/12] update testing and code improvements --- tensorboard/webapp/metrics/actions/index.ts | 2 +- tensorboard/webapp/metrics/internal_types.ts | 2 +- .../webapp/metrics/store/metrics_reducers.ts | 30 ++--- .../webapp/metrics/store/metrics_selectors.ts | 7 +- .../store/metrics_store_internal_utils.ts | 2 +- .../metrics_store_internal_utils_test.ts | 106 ++++++++---------- .../scalar_card_component.ng.html | 1 + .../card_renderer/scalar_card_component.ts | 6 +- .../card_renderer/scalar_card_container.ts | 4 + .../card_renderer/scalar_card_data_table.ts | 2 + .../views/card_renderer/scalar_card_test.ts | 45 +++++++- .../data_table/data_table_component.ng.html | 4 +- .../data_table/data_table_component.scss | 16 +-- .../data_table/data_table_component.ts | 17 +-- .../widgets/data_table/data_table_test.ts | 10 +- 15 files changed, 131 insertions(+), 123 deletions(-) diff --git a/tensorboard/webapp/metrics/actions/index.ts b/tensorboard/webapp/metrics/actions/index.ts index c12221344e..a7f8f52d10 100644 --- a/tensorboard/webapp/metrics/actions/index.ts +++ b/tensorboard/webapp/metrics/actions/index.ts @@ -240,7 +240,7 @@ export const dataTableColumnEdited = createAction( ); export const dataTableColumnToggled = createAction( - '[Metrics] Data table column toggled in edit menu or delete button', + '[Metrics] Data table column toggled in edit menu or delete button clicked', props() ); diff --git a/tensorboard/webapp/metrics/internal_types.ts b/tensorboard/webapp/metrics/internal_types.ts index 144cf83520..c5cc4617e4 100644 --- a/tensorboard/webapp/metrics/internal_types.ts +++ b/tensorboard/webapp/metrics/internal_types.ts @@ -100,9 +100,9 @@ export interface HeaderEditInfo { } export interface HeaderToggleInfo { - dataTableMode?: DataTableMode; headerType: ColumnHeaderType; cardId?: CardId; + dataTableMode?: DataTableMode; } export const SCALARS_SMOOTHING_MIN = 0; diff --git a/tensorboard/webapp/metrics/store/metrics_reducers.ts b/tensorboard/webapp/metrics/store/metrics_reducers.ts index 759968d835..8e3929a636 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers.ts @@ -1376,23 +1376,17 @@ const reducer = createReducer( on( actions.dataTableColumnToggled, (state, {dataTableMode, headerType, cardId}) => { - let rangeSelectionEnabled; - - if (cardId) { - const cardStateMap = state.cardStateMap; - const globalRangeSelectionEnabled = state.rangeSelectionEnabled; - const linkedTimeEnabled = state.linkedTimeEnabled; - rangeSelectionEnabled = cardRangeSelectionEnabled( - cardStateMap, - globalRangeSelectionEnabled, - linkedTimeEnabled, - cardId - ); - } else { - rangeSelectionEnabled = dataTableMode === DataTableMode.RANGE; - } - - const targetedHeaders = rangeSelectionEnabled + const {cardStateMap, rangeSelectionEnabled, linkedTimeEnabled} = state; + const rangeEnabled = cardId + ? cardRangeSelectionEnabled( + cardStateMap, + rangeSelectionEnabled, + linkedTimeEnabled, + cardId + ) + : dataTableMode === DataTableMode.RANGE; + + const targetedHeaders = rangeEnabled ? state.rangeSelectionHeaders : state.singleSelectionHeaders; @@ -1418,7 +1412,7 @@ const reducer = createReducer( enabled: !newHeaders[newToggledHeaderIndex].enabled, }; - if (rangeSelectionEnabled) { + if (rangeEnabled) { return { ...state, rangeSelectionHeaders: newHeaders, diff --git a/tensorboard/webapp/metrics/store/metrics_selectors.ts b/tensorboard/webapp/metrics/store/metrics_selectors.ts index ff6ee2ab2d..a3bbdf3902 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors.ts @@ -485,14 +485,13 @@ export const getMetricsCardRangeSelectionEnabled = createSelector( linkedTimeEnabled: boolean, cardStateMap: CardStateMap, cardId: CardId - ) => { - return cardRangeSelectionEnabled( + ) => + cardRangeSelectionEnabled( cardStateMap, globalRangeSelectionEnabled, linkedTimeEnabled, cardId - ); - } + ) ); /** diff --git a/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts b/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts index b0e0932129..d1b3414093 100644 --- a/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts +++ b/tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts @@ -606,7 +606,7 @@ export function cardRangeSelectionEnabled( globalRangeSelectionEnabled: boolean, linkedTimeEnabled: boolean, cardId: CardId -) { +): boolean { if (linkedTimeEnabled) { return globalRangeSelectionEnabled; } diff --git a/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts b/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts index d81b2bbd4a..5d24c8433d 100644 --- a/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_store_internal_utils_test.ts @@ -1282,97 +1282,81 @@ describe('metrics store utils', () => { describe('cardRangeSelectionEnabled', () => { it('returns card specific value when defined', () => { - let cardStateMap = { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, - }, - }; - let globalRangeSelectionEnabled = false; - let linkedTimeEnabled = false; - let cardId = 'card1'; - expect( cardRangeSelectionEnabled( - cardStateMap, - globalRangeSelectionEnabled, - linkedTimeEnabled, - cardId + { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }, + false, + false, + 'card1' ) ).toBeTrue(); - cardStateMap = { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, - }, - }; - globalRangeSelectionEnabled = true; + expect( cardRangeSelectionEnabled( - cardStateMap, - globalRangeSelectionEnabled, - linkedTimeEnabled, - cardId + { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + }, + true, + false, + 'card1' ) ).toBeFalse(); }); it('returns global value when card specific value is not defined', () => { - let cardStateMap = { - card1: {}, - }; - let globalRangeSelectionEnabled = true; - let linkedTimeEnabled = false; - let cardId = 'card1'; expect( cardRangeSelectionEnabled( - cardStateMap, - globalRangeSelectionEnabled, - linkedTimeEnabled, - cardId + { + card1: {}, + }, + true, + false, + 'card1' ) ).toBeTrue(); - globalRangeSelectionEnabled = false; - expect( cardRangeSelectionEnabled( - cardStateMap, - globalRangeSelectionEnabled, - linkedTimeEnabled, - cardId + { + card1: {}, + }, + false, + false, + 'card1' ) ).toBeFalse(); }); it('returns global value when linked time is enabled', () => { - let cardStateMap = { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, - }, - }; - let globalRangeSelectionEnabled = true; - let linkedTimeEnabled = true; - let cardId = 'card1'; expect( cardRangeSelectionEnabled( - cardStateMap, - globalRangeSelectionEnabled, - linkedTimeEnabled, - cardId + { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + }, + true, + true, + 'card1' ) ).toBeTrue(); - cardStateMap = { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, - }, - }; - globalRangeSelectionEnabled = false; expect( cardRangeSelectionEnabled( - cardStateMap, - globalRangeSelectionEnabled, - linkedTimeEnabled, - cardId + { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }, + false, + true, + 'card1' ) ).toBeFalse(); }); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html index 28b03e2cd0..48e88a11ec 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html @@ -200,6 +200,7 @@ [sortingInfo]="sortingInfo" [columnCustomizationEnabled]="columnCustomizationEnabled" [smoothingEnabled]="smoothingEnabled" + [hparamsEnabled]="hparamsEnabled" (sortDataBy)="sortDataBy($event)" (editColumnHeaders)="editColumnHeaders.emit($event)" (removeColumn)="removeColumn($event)" diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index d00388cacb..910b6b4796 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -107,6 +107,7 @@ export class ScalarCardComponent { @Input() userViewBox!: Extent | null; @Input() columnHeaders!: ColumnHeader[]; @Input() rangeEnabled!: boolean; + @Input() hparamsEnabled!: boolean; @Output() onFullSizeToggle = new EventEmitter(); @Output() onPinClicked = new EventEmitter(); @@ -152,11 +153,10 @@ export class ScalarCardComponent { } removeColumn(headerToggleInfo: HeaderToggleInfo) { - const toggleInfo = { + this.onRemoveColumn.emit({ headerType: headerToggleInfo.headerType, cardId: this.cardId, - }; - this.onRemoveColumn.emit(toggleInfo); + }); } resetDomain() { diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index c91917f9b0..0ffa0a92e8 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -184,6 +184,7 @@ function isMinMaxStepValid(minMax: MinMaxStep | undefined): boolean { [userViewBox]="userViewBox$ | async" [columnHeaders]="columnHeaders$ | async" [rangeEnabled]="rangeEnabled$ | async" + [hparamsEnabled]="hparamsEnabled$ | async" (onFullSizeToggle)="onFullSizeToggle()" (onPinClicked)="pinStateChanged.emit($event)" observeIntersection @@ -234,6 +235,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { stepOrLinkedTimeSelection$?: Observable; cardState$?: Observable>; rangeEnabled$?: Observable; + hparamsEnabled$?: Observable; onVisibilityChange({visible}: {visible: boolean}) { this.isVisible = visible; @@ -597,6 +599,8 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { getMetricsCardRangeSelectionEnabled, this.cardId ); + + this.hparamsEnabled$ = this.store.select(getEnableHparamsInTimeSeries); } ngOnDestroy() { diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts index 11df7428de..6dc005ff7b 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts @@ -46,6 +46,7 @@ import {isDatumVisible} from './utils'; [sortingInfo]="sortingInfo" [columnCustomizationEnabled]="columnCustomizationEnabled" [smoothingEnabled]="smoothingEnabled" + [hparamsEnabled]="hparamsEnabled" (sortDataBy)="sortDataBy.emit($event)" (orderColumns)="orderColumns($event)" (removeColumn)="removeColumn.emit($event)" @@ -61,6 +62,7 @@ export class ScalarCardDataTable { @Input() sortingInfo!: SortingInfo; @Input() columnCustomizationEnabled!: boolean; @Input() smoothingEnabled!: boolean; + @Input() hparamsEnabled!: boolean; @Output() sortDataBy = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index 1bfc3dbc3c..02c44216ce 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -4308,7 +4308,7 @@ describe('scalar card', () => { ]); })); - it('emits dataTableColumnToggled when onRemoveColumn is called', fakeAsync(() => { + it('emits dataTableColumnToggled when onRemoveColumn is called with range selection disabled', fakeAsync(() => { store.overrideSelector(getSingleSelectionHeaders, [ { type: ColumnHeaderType.RUN, @@ -4325,17 +4325,16 @@ describe('scalar card', () => { ]); store.overrideSelector(getCardStateMap, { card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, }, }); const fixture = createComponent('card1'); fixture.detectChanges(); - const headerToggleInfo = { + fixture.componentInstance.onRemoveColumn({ cardId: 'card1', headerType: ColumnHeaderType.RUN, - }; - fixture.componentInstance.onRemoveColumn(headerToggleInfo); + }); expect(dispatchedActions).toEqual([ dataTableColumnToggled({ @@ -4344,6 +4343,42 @@ describe('scalar card', () => { }), ]); })); + + it('emits dataTableColumnToggled when onRemoveColumn is called with range selection enabled', fakeAsync(() => { + store.overrideSelector(getRangeSelectionHeaders, [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.MIN_VALUE, + name: 'minValue', + displayName: 'Min Value', + enabled: true, + }, + ]); + store.overrideSelector(getCardStateMap, { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }); + const fixture = createComponent('card1'); + fixture.detectChanges(); + + fixture.componentInstance.onRemoveColumn({ + cardId: 'card1', + headerType: ColumnHeaderType.MIN_VALUE, + }); + + expect(dispatchedActions).toEqual([ + dataTableColumnToggled({ + cardId: 'card1', + headerType: ColumnHeaderType.MIN_VALUE, + }), + ]); + })); }); }); diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index 07176dd966..96f8985519 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -42,11 +42,11 @@ svgIcon="arrow_downward_24px" >
-
+
diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.scss b/tensorboard/webapp/widgets/data_table/data_table_component.scss index 50e29f155e..885bcc7561 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.scss +++ b/tensorboard/webapp/widgets/data_table/data_table_component.scss @@ -87,16 +87,10 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); width: 12px; } - .sorting-icon-container { - width: 12px; - height: 12px; - border-radius: 5px; - } - .delete-icon-container { - width: 12px; - height: 12px; border-radius: 5px; + height: 12px; + width: 12px; } .delete-icon-container:hover { @@ -104,6 +98,12 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); cursor: pointer; } + .sorting-icon-container { + border-radius: 5px; + height: 12px; + width: 12px; + } + .show { opacity: 1; } diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ts b/tensorboard/webapp/widgets/data_table/data_table_component.ts index f10daf9542..52f46cefe4 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ts @@ -19,7 +19,6 @@ import { EventEmitter, Input, OnDestroy, - OnInit, Output, } from '@angular/core'; import { @@ -34,10 +33,6 @@ import { numberFormatter, relativeTimeFormatter, } from '../line_chart_v2/lib/formatter'; -import {getEnableHparamsInTimeSeries} from '../../feature_flag/store/feature_flag_selectors'; -import {BehaviorSubject} from 'rxjs'; -import {Store} from '@ngrx/store'; -import {State} from '../../app_state'; enum Side { RIGHT, @@ -54,7 +49,7 @@ const preventDefault = function (e: MouseEvent) { styleUrls: ['data_table_component.css'], changeDetection: ChangeDetectionStrategy.OnPush, }) -export class DataTableComponent implements OnInit, OnDestroy { +export class DataTableComponent implements OnDestroy { // The order of this array of headers determines the order which they are // displayed in the table. @Input() headers!: ColumnHeader[]; @@ -62,6 +57,7 @@ export class DataTableComponent implements OnInit, OnDestroy { @Input() sortingInfo!: SortingInfo; @Input() columnCustomizationEnabled!: boolean; @Input() smoothingEnabled!: boolean; + @Input() hparamsEnabled!: boolean; @Output() sortDataBy = new EventEmitter(); @Output() orderColumns = new EventEmitter(); @@ -76,20 +72,11 @@ export class DataTableComponent implements OnInit, OnDestroy { draggingHeaderName: string | undefined; highlightedColumnName: string | undefined; highlightSide: Side = Side.RIGHT; - HParamsEnabled = new BehaviorSubject(false); - - constructor(private readonly store: Store) {} ngOnDestroy() { document.removeEventListener('dragover', preventDefault); } - ngOnInit() { - this.store.select(getEnableHparamsInTimeSeries).subscribe((enabled) => { - this.HParamsEnabled.next(enabled); - }); - } - getFormattedDataForColumn( columnHeader: ColumnHeaderType, datum: string | number | undefined diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts index 4ec6bb0ca4..1c07249722 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_test.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts @@ -26,7 +26,6 @@ import { } from './types'; import {DataTableComponent} from './data_table_component'; import {DataTableModule} from './data_table_module'; -import {getEnableHparamsInTimeSeries} from '../../feature_flag/store/feature_flag_selectors'; import {MockStore} from '@ngrx/store/testing'; import {State} from '../../app_state'; import {Store} from '@ngrx/store'; @@ -41,6 +40,7 @@ import {provideMockTbStore} from '../../testing/utils'; [data]="data" [sortingInfo]="sortingInfo" [smoothingEnabled]="smoothingEnabled" + [hparamsEnabled]="hparamsEnabled" (sortDataBy)="sortDataBy($event)" (orderColumns)="orderColumns($event)" (removeColumn)="removeColumn($event)" @@ -55,6 +55,7 @@ class TestableComponent { @Input() data!: TableData[]; @Input() sortingInfo!: SortingInfo; @Input() smoothingEnabled!: boolean; + @Input() hparamsEnabled!: boolean; @Input() sortDataBy!: (sortingInfo: SortingInfo) => void; @Input() orderColumns!: (newOrder: ColumnHeaderType[]) => void; @@ -80,6 +81,7 @@ describe('data table', () => { data?: TableData[]; sortingInfo?: SortingInfo; smoothingEnabled?: boolean; + hparamsEnabled?: boolean; }): ComponentFixture { const fixture = TestBed.createComponent(TestableComponent); @@ -806,7 +808,6 @@ describe('data table', () => { describe('delete column button', () => { it('emits removeColumn event when delete button clicked', () => { - store.overrideSelector(getEnableHparamsInTimeSeries, true); const fixture = createComponent({ headers: [ { @@ -835,6 +836,7 @@ describe('data table', () => { }, ], }); + fixture.componentInstance.hparamsEnabled = true; fixture.detectChanges(); const headerElements = fixture.debugElement.queryAll( By.css('.header > .col') @@ -849,7 +851,6 @@ describe('data table', () => { }); it('renders delete button when hparam flag is on', () => { - store.overrideSelector(getEnableHparamsInTimeSeries, true); const fixture = createComponent({ headers: [ { @@ -878,6 +879,7 @@ describe('data table', () => { }, ], }); + fixture.componentInstance.hparamsEnabled = true; fixture.detectChanges(); const headerElements = fixture.debugElement.queryAll( By.css('.header > .col') @@ -887,7 +889,6 @@ describe('data table', () => { }); it('does not render delete button when hparam flag is off', () => { - store.overrideSelector(getEnableHparamsInTimeSeries, false); const fixture = createComponent({ headers: [ { @@ -916,6 +917,7 @@ describe('data table', () => { }, ], }); + fixture.componentInstance.hparamsEnabled = false; fixture.detectChanges(); const headerElements = fixture.debugElement.queryAll( By.css('.header > .col') From f0cd295932e08ba814be6b0d7099ddd29c080416 Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Tue, 6 Jun 2023 22:07:04 +0000 Subject: [PATCH 08/12] optional params and remove unnecessary imports --- tensorboard/webapp/metrics/store/metrics_selectors.ts | 4 ++-- .../metrics/views/card_renderer/scalar_card_component.ts | 2 +- .../views/card_renderer/scalar_card_data_table.ts | 2 +- tensorboard/webapp/widgets/data_table/BUILD | 9 --------- .../webapp/widgets/data_table/data_table_component.ts | 2 +- tensorboard/webapp/widgets/data_table/data_table_test.ts | 7 ------- 6 files changed, 5 insertions(+), 21 deletions(-) diff --git a/tensorboard/webapp/metrics/store/metrics_selectors.ts b/tensorboard/webapp/metrics/store/metrics_selectors.ts index a3bbdf3902..0e35780e6d 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors.ts @@ -477,13 +477,13 @@ export const getTableEditorSelectedTab = createSelector( ); export const getMetricsCardRangeSelectionEnabled = createSelector( + getCardStateMap, getMetricsRangeSelectionEnabled, getMetricsLinkedTimeEnabled, - getCardStateMap, ( + cardStateMap: CardStateMap, globalRangeSelectionEnabled: boolean, linkedTimeEnabled: boolean, - cardStateMap: CardStateMap, cardId: CardId ) => cardRangeSelectionEnabled( diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index 910b6b4796..f1dd1c177b 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -107,7 +107,7 @@ export class ScalarCardComponent { @Input() userViewBox!: Extent | null; @Input() columnHeaders!: ColumnHeader[]; @Input() rangeEnabled!: boolean; - @Input() hparamsEnabled!: boolean; + @Input() hparamsEnabled?: boolean; @Output() onFullSizeToggle = new EventEmitter(); @Output() onPinClicked = new EventEmitter(); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts index 6dc005ff7b..c7b68a36a8 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts @@ -62,7 +62,7 @@ export class ScalarCardDataTable { @Input() sortingInfo!: SortingInfo; @Input() columnCustomizationEnabled!: boolean; @Input() smoothingEnabled!: boolean; - @Input() hparamsEnabled!: boolean; + @Input() hparamsEnabled?: boolean; @Output() sortDataBy = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); diff --git a/tensorboard/webapp/widgets/data_table/BUILD b/tensorboard/webapp/widgets/data_table/BUILD index 5f4c43788e..68b2b9607c 100644 --- a/tensorboard/webapp/widgets/data_table/BUILD +++ b/tensorboard/webapp/widgets/data_table/BUILD @@ -35,15 +35,11 @@ tf_ng_module( deps = [ ":data_table_header", ":types", - "//tensorboard/webapp:app_state", "//tensorboard/webapp/angular:expect_angular_material_icon", - "//tensorboard/webapp/feature_flag/store", "//tensorboard/webapp/metrics/views/card_renderer:scalar_card_types", "//tensorboard/webapp/widgets/line_chart_v2/lib:formatter", "@npm//@angular/common", "@npm//@angular/core", - "@npm//@ngrx/store", - "@npm//rxjs", ], ) @@ -81,15 +77,10 @@ tf_ts_library( deps = [ ":data_table", ":types", - "//tensorboard/webapp:app_state", "//tensorboard/webapp/angular:expect_angular_core_testing", "//tensorboard/webapp/angular:expect_angular_material_icon", - "//tensorboard/webapp/feature_flag/store", - "//tensorboard/webapp/testing:utils", "@npm//@angular/core", "@npm//@angular/platform-browser", - "@npm//@ngrx/store", "@npm//@types/jasmine", - "@npm//rxjs", ], ) diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ts b/tensorboard/webapp/widgets/data_table/data_table_component.ts index 52f46cefe4..58498326ea 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ts @@ -57,7 +57,7 @@ export class DataTableComponent implements OnDestroy { @Input() sortingInfo!: SortingInfo; @Input() columnCustomizationEnabled!: boolean; @Input() smoothingEnabled!: boolean; - @Input() hparamsEnabled!: boolean; + @Input() hparamsEnabled?: boolean = false; @Output() sortDataBy = new EventEmitter(); @Output() orderColumns = new EventEmitter(); diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts index 1c07249722..311bc982e4 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_test.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts @@ -26,10 +26,6 @@ import { } from './types'; import {DataTableComponent} from './data_table_component'; import {DataTableModule} from './data_table_module'; -import {MockStore} from '@ngrx/store/testing'; -import {State} from '../../app_state'; -import {Store} from '@ngrx/store'; -import {provideMockTbStore} from '../../testing/utils'; @Component({ selector: 'testable-comp', @@ -63,7 +59,6 @@ class TestableComponent { } describe('data table', () => { - let store: MockStore; let sortDataBySpy: jasmine.Spy; let orderColumnsSpy: jasmine.Spy; let removeColumnSpy: jasmine.Spy; @@ -71,9 +66,7 @@ describe('data table', () => { await TestBed.configureTestingModule({ declarations: [TestableComponent, DataTableComponent], imports: [MatIconModule, DataTableModule], - providers: [provideMockTbStore()], }).compileComponents(); - store = TestBed.inject>(Store) as MockStore; }); function createComponent(input: { From bcd2ef5279966ad3c5bb95cfdd107d892a18e77d Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Wed, 7 Jun 2023 21:07:25 +0000 Subject: [PATCH 09/12] button position/styling --- .../data_table/data_table_component.ng.html | 16 +++++----- .../data_table/data_table_component.scss | 15 ++++++++++ .../widgets/data_table/data_table_test.ts | 30 +++++++++---------- 3 files changed, 38 insertions(+), 23 deletions(-) diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index 96f8985519..91be258a2d 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -29,6 +29,14 @@ class="cell" [ngClass]="getHeaderHighlightStyle(header.name)" > +
+ + +
-
- - -
diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.scss b/tensorboard/webapp/widgets/data_table/data_table_component.scss index 885bcc7561..6df8b5af18 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.scss +++ b/tensorboard/webapp/widgets/data_table/data_table_component.scss @@ -87,12 +87,27 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); width: 12px; } + .col:hover .delete-icon-container { + opacity: 1; + } + .delete-icon-container { border-radius: 5px; height: 12px; + margin-left: -12px; + opacity: 0; + position: absolute; width: 12px; } + .delete-icon { + color: mat.get-color-from-palette(mat.$gray-palette, 500); + } + + .delete-icon:hover { + color: #fff; + } + .delete-icon-container:hover { background-color: mat.get-color-from-palette(mat.$gray-palette, 300); cursor: pointer; diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts index 311bc982e4..1db1030208 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_test.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts @@ -508,32 +508,32 @@ describe('data table', () => { expect( headerElements[1] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show') ).toBe(true); expect( headerElements[1] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.getAttribute('svgIcon') ).toBe('arrow_upward_24px'); expect( headerElements[2] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show') ).toBe(false); expect( headerElements[2] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show-on-hover') ).toBe(true); expect( headerElements[3] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show') ).toBe(false); expect( headerElements[3] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show-on-hover') ).toBe(true); }); @@ -572,32 +572,32 @@ describe('data table', () => { expect( headerElements[1] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show') ).toBe(false); expect( headerElements[1] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show-on-hover') ).toBe(true); expect( headerElements[2] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show') ).toBe(false); expect( headerElements[2] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show-on-hover') ).toBe(true); expect( headerElements[3] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.classList.contains('show') ).toBe(true); expect( headerElements[3] - .queryAll(By.css('mat-icon'))[0] + .query(By.css('.sorting-icon-container mat-icon')) .nativeElement.getAttribute('svgIcon') ).toBe('arrow_downward_24px'); }); @@ -836,7 +836,7 @@ describe('data table', () => { ); headerElements[3] - .queryAll(By.css('mat-icon'))[1] + .query(By.css('.delete-icon')) .triggerEventHandler('click', {}); expect(removeColumnSpy).toHaveBeenCalledOnceWith({ headerType: ColumnHeaderType.STEP, @@ -878,7 +878,7 @@ describe('data table', () => { By.css('.header > .col') ); - expect(headerElements[3].queryAll(By.css('mat-icon'))[1]).toBeTruthy(); + expect(headerElements[3].query(By.css('.delete-icon'))).toBeTruthy(); }); it('does not render delete button when hparam flag is off', () => { @@ -916,7 +916,7 @@ describe('data table', () => { By.css('.header > .col') ); - expect(headerElements[3].queryAll(By.css('mat-icon'))[1]).toBeFalsy(); + expect(headerElements[3].query(By.css('.delete-icon'))).toBeFalsy(); }); }); }); From c3c2d0cd909d745e97c6309690004a0ffdeb597e Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Wed, 7 Jun 2023 21:12:35 +0000 Subject: [PATCH 10/12] test line spacing --- tensorboard/webapp/widgets/data_table/data_table_test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts index 1db1030208..7d106bf6dc 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_test.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts @@ -831,13 +831,14 @@ describe('data table', () => { }); fixture.componentInstance.hparamsEnabled = true; fixture.detectChanges(); + const headerElements = fixture.debugElement.queryAll( By.css('.header > .col') ); - headerElements[3] .query(By.css('.delete-icon')) .triggerEventHandler('click', {}); + expect(removeColumnSpy).toHaveBeenCalledOnceWith({ headerType: ColumnHeaderType.STEP, }); @@ -874,6 +875,7 @@ describe('data table', () => { }); fixture.componentInstance.hparamsEnabled = true; fixture.detectChanges(); + const headerElements = fixture.debugElement.queryAll( By.css('.header > .col') ); @@ -912,6 +914,7 @@ describe('data table', () => { }); fixture.componentInstance.hparamsEnabled = false; fixture.detectChanges(); + const headerElements = fixture.debugElement.queryAll( By.css('.header > .col') ); From a49ebb760fe527960d4d8b20680ac39d03775e7d Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Wed, 7 Jun 2023 22:19:14 +0000 Subject: [PATCH 11/12] combine button css --- .../webapp/widgets/data_table/data_table_component.scss | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.scss b/tensorboard/webapp/widgets/data_table/data_table_component.scss index 6df8b5af18..f7e4b6dfbd 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.scss +++ b/tensorboard/webapp/widgets/data_table/data_table_component.scss @@ -93,6 +93,7 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); .delete-icon-container { border-radius: 5px; + color: mat.get-color-from-palette(mat.$gray-palette, 500); height: 12px; margin-left: -12px; opacity: 0; @@ -100,10 +101,6 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); width: 12px; } - .delete-icon { - color: mat.get-color-from-palette(mat.$gray-palette, 500); - } - .delete-icon:hover { color: #fff; } From aa07cffaaf8f4b8b28492ac549e1290fc78e5bd1 Mon Sep 17 00:00:00 2001 From: bhuang1323 Date: Fri, 9 Jun 2023 22:41:42 +0000 Subject: [PATCH 12/12] improve testing --- .../metrics/store/metrics_reducers_test.ts | 1 + .../data_table/header_cell_component.scss | 4 ++-- .../data_table/header_cell_component_test.ts | 20 +++++++++---------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts index 8c5a5227c1..55f927ccd7 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts @@ -1965,6 +1965,7 @@ describe('metrics reducers', () => { describe('dataTableColumnToggled', () => { let beforeState: MetricsState; + beforeEach(() => { beforeState = buildMetricsState({ rangeSelectionHeaders: [ diff --git a/tensorboard/webapp/widgets/data_table/header_cell_component.scss b/tensorboard/webapp/widgets/data_table/header_cell_component.scss index 873ab558bf..e2c8a6e25e 100644 --- a/tensorboard/webapp/widgets/data_table/header_cell_component.scss +++ b/tensorboard/webapp/widgets/data_table/header_cell_component.scss @@ -47,13 +47,13 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); } .delete-icon-container { - width: 12px; - height: 12px; border-radius: 5px; color: mat.get-color-from-palette(mat.$gray-palette, 500); + height: 12px; margin-left: -12px; opacity: 0; position: absolute; + width: 12px; } .delete-icon-container:hover { diff --git a/tensorboard/webapp/widgets/data_table/header_cell_component_test.ts b/tensorboard/webapp/widgets/data_table/header_cell_component_test.ts index 626639c290..7e8273134e 100644 --- a/tensorboard/webapp/widgets/data_table/header_cell_component_test.ts +++ b/tensorboard/webapp/widgets/data_table/header_cell_component_test.ts @@ -106,11 +106,14 @@ describe('header cell', () => { }); describe('delete column button', () => { - it('emits removeColumn event when delete button clicked', () => { - const fixture = createComponent({}); + let fixture: ComponentFixture; + beforeEach(() => { + fixture = createComponent({}); fixture.componentInstance.hparamsEnabled = true; fixture.detectChanges(); + }); + it('emits removeColumn event when delete button clicked', () => { fixture.debugElement .query(By.directive(HeaderCellComponent)) .componentInstance.deleteButtonClicked.subscribe(); @@ -126,20 +129,15 @@ describe('header cell', () => { }); }); - it('renders delete button when hparam flag is on', () => { - const fixture = createComponent({}); - fixture.componentInstance.hparamsEnabled = true; - fixture.detectChanges(); - + it('renders delete button when hparamsEnabled is true', () => { expect(fixture.debugElement.query(By.css('.delete-icon'))).toBeTruthy(); }); - it('does not render delete button when hparam flag is off', () => { - const fixture = createComponent({}); - fixture.componentInstance.hparamsEnabled = true; + it('does not render delete button when hparamsEnabled is false', () => { + fixture.componentInstance.hparamsEnabled = false; fixture.detectChanges(); - expect(fixture.debugElement.query(By.css('.delete-icon'))).toBeTruthy(); + expect(fixture.debugElement.query(By.css('.delete-icon'))).toBeFalsy(); }); }); });