From 56c51b03a456bb7f448b8fe783971d5addbceb99 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 15 Jun 2023 08:11:43 -0700 Subject: [PATCH] HParams: project color header (#6434) ## Motivation for features / changes In #6422 we started projecting the header components into the data table widget. However, the content was still not projected and the color column was hard coded in. Therefore we left a hard coded color header. The content started being projected in #6427. Therefore we can stop hardcoding this color column. This PR removes the hardcoded header column and starts adding it from the ScalarCardDataTable ## Technical description of changes The color header cannot be sorted, dismissed, or dragged. There was some logic in place for this. However, it was incomplete. This completes that logic and uses it for the color header. We also add tests for that logic which were not in place before. Note: I did some test clean up in the HeaderCell ## Screenshot of changes It still works. Screenshot 2023-06-15 at 5 41 33 AM ComponentTests for consistency. --------- Co-authored-by: Riley Jones <78179109+rileyajones@users.noreply.github.com> --- .../scalar_card_data_table.ng.html | 3 +- .../views/card_renderer/scalar_card_test.ts | 9 +- .../data_table/data_table_component.ng.html | 1 - .../data_table/header_cell_component.ng.html | 6 +- .../data_table/header_cell_component.ts | 2 +- .../data_table/header_cell_component_test.ts | 100 +++++++++++++++--- 6 files changed, 100 insertions(+), 21 deletions(-) diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html index ed41d9bf31..3a563ba18c 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html @@ -24,12 +24,13 @@ (removeColumn)="removeColumn.emit($event)" > - + 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 141304ecc7..a8f90a2f09 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -2649,7 +2649,7 @@ describe('scalar card', () => { expect(dataTableComponent).toBeFalsy(); })); - it('projects tb-data-table-header-cell for enabled headers', fakeAsync(() => { + it('projects tb-data-table-header-cell for color and enabled headers', fakeAsync(() => { store.overrideSelector(getMetricsLinkedTimeSelection, { start: {step: 20}, end: null, @@ -2681,13 +2681,16 @@ describe('scalar card', () => { By.directive(DataTableComponent) ).componentInstance; - expect(dataTableComponentInstance.headerCells.length).toEqual(2); + expect(dataTableComponentInstance.headerCells.length).toEqual(3); expect( dataTableComponentInstance.headerCells.get(0).header.name - ).toEqual('run'); + ).toEqual('color'); expect( dataTableComponentInstance.headerCells.get(1).header.name + ).toEqual('run'); + expect( + dataTableComponentInstance.headerCells.get(2).header.name ).toEqual('step'); })); 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 ef73110b42..1bf0b8fc2b 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -14,7 +14,6 @@
-
diff --git a/tensorboard/webapp/widgets/data_table/header_cell_component.ng.html b/tensorboard/webapp/widgets/data_table/header_cell_component.ng.html index 4cef679dcf..e99b8fc59d 100644 --- a/tensorboard/webapp/widgets/data_table/header_cell_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/header_cell_component.ng.html @@ -14,7 +14,7 @@
-
+
-
+
@@ -44,7 +45,8 @@ class TestableComponent { @Input() header!: ColumnHeader; @Input() sortingInfo!: SortingInfo; - @Input() hparamsEnabled?: boolean; + @Input() hparamsEnabled!: boolean; + @Input() controlsEnabled!: boolean; @Input() headerClicked!: (sortingInfo: SortingInfo) => void; @Input() deleteButtonClicked!: (header: ColumnHeader) => void; @@ -63,6 +65,8 @@ describe('header cell', () => { function createComponent(input: { header?: ColumnHeader; sortingInfo?: SortingInfo; + hparamsEnabled?: boolean; + controlsEnabled?: boolean; }): ComponentFixture { const fixture = TestBed.createComponent(TestableComponent); @@ -76,6 +80,8 @@ describe('header cell', () => { name: 'run', order: SortingOrder.ASCENDING, }; + fixture.componentInstance.hparamsEnabled = input.hparamsEnabled ?? true; + fixture.componentInstance.controlsEnabled = input.controlsEnabled ?? true; headerClickedSpy = jasmine.createSpy(); fixture.componentInstance.headerClicked = headerClickedSpy; @@ -83,19 +89,18 @@ describe('header cell', () => { deleteButtonClickedSpy = jasmine.createSpy(); fixture.componentInstance.deleteButtonClicked = deleteButtonClickedSpy; + fixture.detectChanges(); return fixture; } it('renders', () => { const fixture = createComponent({}); - fixture.detectChanges(); const cell = fixture.debugElement.query(By.css('.cell')); expect(cell).toBeTruthy(); }); it('emits headerClicked event when cell element is clicked', () => { const fixture = createComponent({}); - fixture.detectChanges(); fixture.debugElement .query(By.directive(HeaderCellComponent)) .componentInstance.headerClicked.subscribe(); @@ -104,14 +109,8 @@ describe('header cell', () => { }); describe('delete column button', () => { - let fixture: ComponentFixture; - beforeEach(() => { - fixture = createComponent({}); - fixture.componentInstance.hparamsEnabled = true; - fixture.detectChanges(); - }); - it('emits removeColumn event when delete button clicked', () => { + const fixture = createComponent({hparamsEnabled: true}); fixture.debugElement .query(By.directive(HeaderCellComponent)) .componentInstance.deleteButtonClicked.subscribe(); @@ -128,14 +127,91 @@ describe('header cell', () => { }); it('renders delete button when hparamsEnabled is true', () => { + const fixture = createComponent({hparamsEnabled: true}); + expect(fixture.debugElement.query(By.css('.delete-icon'))).toBeTruthy(); }); it('does not render delete button when hparamsEnabled is false', () => { - fixture.componentInstance.hparamsEnabled = false; - fixture.detectChanges(); + const fixture = createComponent({hparamsEnabled: false}); expect(fixture.debugElement.query(By.css('.delete-icon'))).toBeFalsy(); }); + + it('does not render delete button when controlsEnabled is false', () => { + const fixture = createComponent({controlsEnabled: false}); + + expect(fixture.debugElement.query(By.css('.delete-icon'))).toBeFalsy(); + }); + }); + + describe('sorting icon', () => { + it('shows sorting icon when sortingInfo matches header', () => { + const fixture = createComponent({ + sortingInfo: { + name: 'run', + order: SortingOrder.ASCENDING, + }, + }); + + expect( + fixture.debugElement + .query(By.css('.sorting-icon-container mat-icon')) + .nativeElement.classList.contains('show') + ).toBe(true); + }); + + it('does not render sorting icon when sortingInfo does not match header', () => { + const fixture = createComponent({ + sortingInfo: { + name: 'not-this-header', + order: SortingOrder.ASCENDING, + }, + }); + + expect( + fixture.debugElement + .query(By.css('.sorting-icon-container mat-icon')) + .nativeElement.classList.contains('show') + ).toBe(false); + }); + + it('renders downward arrow if order is DESCENDING', () => { + const fixture = createComponent({ + sortingInfo: { + name: 'run', + order: SortingOrder.DESCENDING, + }, + }); + + expect( + fixture.debugElement + .query(By.css('.sorting-icon-container mat-icon')) + .nativeElement.getAttribute('svgIcon') + ).toBe('arrow_downward_24px'); + }); + + it('renders downward arrow if order is DESCENDING', () => { + const fixture = createComponent({ + sortingInfo: { + name: 'run', + order: SortingOrder.ASCENDING, + }, + }); + + expect( + fixture.debugElement + .query(By.css('.sorting-icon-container mat-icon')) + .nativeElement.getAttribute('svgIcon') + ).toBe('arrow_upward_24px'); + }); + + it('does not render sorting icon when controlsEnabled is false', () => { + const fixture = createComponent({controlsEnabled: false}); + + expect( + fixture.debugElement.query(By.css('.sorting-icon-container mat-icon')) + ).toBeFalsy(); + }); }); });