Skip to content

Commit

Permalink
HParams: project color header (#6434)
Browse files Browse the repository at this point in the history
## 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.
<img width="148" alt="Screenshot 2023-06-15 at 5 41 33 AM"
src="https://github.com/tensorflow/tensorboard/assets/8672809/0fe6e62e-f79a-46a8-bf60-cad3d87a28d4">
ComponentTests for consistency.

---------

Co-authored-by: Riley Jones <78179109+rileyajones@users.noreply.github.com>
  • Loading branch information
JamesHollyer and rileyajones authored Jun 15, 2023
1 parent 0848088 commit 56c51b0
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@
(removeColumn)="removeColumn.emit($event)"
>
<ng-container header>
<ng-container *ngFor="let header of columnHeaders">
<ng-container *ngFor="let header of getHeaders()">
<tb-data-table-header-cell
*ngIf="header.enabled && (header.type !== ColumnHeaderType.SMOOTHED || smoothingEnabled)"
[header]="header"
[sortingInfo]="sortingInfo"
[hparamsEnabled]="hparamsEnabled"
[controlsEnabled]="header.type !== ColumnHeaderType.COLOR"
></tb-data-table-header-cell> </ng-container
></ng-container>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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');
}));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

<div class="data-table">
<div class="header">
<div class="col"></div>
<ng-content select="[header]"></ng-content>
</div>
<ng-content select="[content]"></ng-content>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@

<div
class="cell"
[draggable]="sortable"
[draggable]="controlsEnabled"
(dragstart)="dragStart.emit(header)"
(dragend)="dragEnd.emit()"
(dragenter)="dragEnter.emit(header)"
(click)="headerClicked.emit(header.name)"
[ngClass]="highlightStyle$ | async"
>
<ng-content></ng-content>
<div *ngIf="hparamsEnabled" class="delete-icon-container">
<div *ngIf="hparamsEnabled && controlsEnabled" class="delete-icon-container">
<button
class="delete-icon"
mat-icon-button
Expand All @@ -34,7 +34,7 @@
</button>
</div>
<tb-data-table-header [header]="header"></tb-data-table-header>
<div *ngIf="sortable" class="sorting-icon-container">
<div *ngIf="controlsEnabled" class="sorting-icon-container">
<mat-icon
*ngIf="
sortingInfo.order === SortingOrder.ASCENDING ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import {BehaviorSubject} from 'rxjs';
export class HeaderCellComponent {
@Input() header!: ColumnHeader;
@Input() sortingInfo!: SortingInfo;
@Input() sortable: boolean = true;
@Input() controlsEnabled: boolean = true;
@Input() hparamsEnabled?: boolean = false;

@Output() dragStart = new EventEmitter<ColumnHeader>();
Expand Down
100 changes: 88 additions & 12 deletions tensorboard/webapp/widgets/data_table/header_cell_component_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {HeaderCellComponent} from './header_cell_component';
[header]="header"
[sortingInfo]="sortingInfo"
[hparamsEnabled]="hparamsEnabled"
[controlsEnabled]="controlsEnabled"
(headerClicked)="headerClicked($event)"
(deleteButtonClicked)="deleteButtonClicked($event)"
></tb-data-table-header-cell>
Expand All @@ -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;
Expand All @@ -63,6 +65,8 @@ describe('header cell', () => {
function createComponent(input: {
header?: ColumnHeader;
sortingInfo?: SortingInfo;
hparamsEnabled?: boolean;
controlsEnabled?: boolean;
}): ComponentFixture<TestableComponent> {
const fixture = TestBed.createComponent(TestableComponent);

Expand All @@ -76,26 +80,27 @@ 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;

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();
Expand All @@ -104,14 +109,8 @@ describe('header cell', () => {
});

describe('delete column button', () => {
let fixture: ComponentFixture<TestableComponent>;
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();
Expand All @@ -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();
});
});
});

0 comments on commit 56c51b0

Please sign in to comment.