HParams: Header Button Styling and Context Menu updates (#6471)

## Motivation for features / changes
While testing the new hparams in time series feature we got a lot of
feedback about the styling and placement of the header buttons.

The main takeaways are:
- Remove the "Delete Column" button
- Ensure the icons are the same height and color
- The 3dot button ripple was weird

We also noticed that the sort button was missing from the context menu
and discovered that right clicking a header cell logged an error to the
console

## Screenshots of UI changes (or N/A)

Header Buttons (Dark Mode)

![image](https://github.com/tensorflow/tensorboard/assets/78179109/cd26c82d-12ad-4915-b476-8bf72a2c34bb)

Header Buttons (Light Mode)

![image](https://github.com/tensorflow/tensorboard/assets/78179109/e33e6dd8-ff0e-4ae8-9726-dd931353ffcc)

Context Menu (Dark Mode)

![image](https://github.com/tensorflow/tensorboard/assets/78179109/5fcb762a-539b-43f0-b044-b73f0e55aa79)

Context Menu (Light Mode)

![image](https://github.com/tensorflow/tensorboard/assets/78179109/4a050bc9-6465-48db-8c7b-fb2348c85e74)

No context menu in scalar card

![image](https://github.com/tensorflow/tensorboard/assets/78179109/6f0220a9-5213-4cca-aab7-832be9f4f5a6)

Author: rileyajones

tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html

@@ -30,6 +30,7 @@
         [header]="header"
         [sortingInfo]="sortingInfo"
         [hparamsEnabled]="hparamsEnabled"
+        disableContextMenu="true"
       ></tb-data-table-header-cell> </ng-container
   ></ng-container>
 

tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html

@@ -40,7 +40,6 @@
         [header]="header"
         [sortingInfo]="sortingInfo"
         [hparamsEnabled]="true"
-        (contextmenu)="openContextMenu($event, header)"
       >
         <ng-container [ngSwitch]="header.name">
           <div *ngSwitchCase="'selected'">

tensorboard/webapp/widgets/data_table/data_table_component.ng.html

@@ -15,19 +15,37 @@
 <custom-modal #contextMenu>
   <div class="context-menu">
     <div
-      *ngIf="!canContextMenuRemoveColumn() && !(selectableColumns && selectableColumns.length)"
+      *ngIf="!contextMenuHeader?.removable && !contextMenuHeader?.sortable && !canContextMenuInsert()"
       class="no-actions-message"
     >
       No Actions Available
     </div>
     <button
-      *ngIf="canContextMenuRemoveColumn()"
+      *ngIf="contextMenuHeader?.removable"
       class="context-menu-button"
       mat-button
       (click)="contextMenuRemoveColumn()"
     >
       <mat-icon svgIcon="close_24px"></mat-icon>Remove
     </button>
+    <button
+      *ngIf="contextMenuHeader?.sortable"
+      class="context-menu-button sort-button"
+      mat-button
+      (click)="sortByHeader(contextMenuHeader?.name)"
+    >
+      <ng-container
+        *ngIf="
+          sortingInfo.order === SortingOrder.ASCENDING &&
+          sortingInfo.name === contextMenuHeader?.name;
+          else descending"
+      >
+        <mat-icon svgIcon="arrow_downward_24px"></mat-icon>Sort Descending
+      </ng-container>
+      <ng-template #descending>
+        <mat-icon svgIcon="arrow_upward_24px"></mat-icon>Sort Ascending
+      </ng-template>
+    </button>
     <button
       mat-button
       *ngIf="canContextMenuInsert()"

tensorboard/webapp/widgets/data_table/data_table_component.scss

@@ -73,4 +73,10 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
     padding: 8px;
     text-wrap: nowrap;
   }
+
+  .sort-button {
+    ::ng-deep path {
+      fill: unset;
+    }
+  }
 }

tensorboard/webapp/widgets/data_table/data_table_component.ts

@@ -120,10 +120,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit {
         headerCell.dragStart.subscribe(this.dragStart.bind(this)),
         headerCell.dragEnter.subscribe(this.dragEnter.bind(this)),
         headerCell.dragEnd.subscribe(this.dragEnd.bind(this)),
-        headerCell.headerClicked.subscribe(this.headerClicked.bind(this)),
-        headerCell.deleteButtonClicked.subscribe(
-          this.deleteButtonClicked.bind(this)
-        ),
+        headerCell.headerClicked.subscribe(this.sortByHeader.bind(this)),
         headerCell.contextMenuOpened.subscribe(
           this.openContextMenu.bind(this, headerCell.header)
         )
@@ -144,7 +141,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit {
       .flat();
   }
 
-  headerClicked(name: string) {
+  sortByHeader(name: string) {
     if (
       this.sortingInfo.name === name &&
       this.sortingInfo.order === SortingOrder.ASCENDING
@@ -236,10 +233,6 @@ export class DataTableComponent implements OnDestroy, AfterContentInit {
     });
   }
 
-  deleteButtonClicked(header: ColumnHeader) {
-    this.removeColumn.emit(header);
-  }
-
   focusColumnSelector() {
     this.columnSelector.focus();
   }

tensorboard/webapp/widgets/data_table/data_table_test.ts

@@ -552,6 +552,7 @@ describe('data table', () => {
           displayName: 'Run',
           enabled: true,
           movable: true,
+          sortable: true,
         },
         {
           name: 'disabled_header',
@@ -577,6 +578,12 @@ describe('data table', () => {
           removable: true,
           movable: false,
         },
+        {
+          name: 'some static column',
+          type: ColumnHeaderType.HPARAM,
+          displayName: 'cant touch this',
+          enabled: true,
+        },
       ];
       mockTableData = [
         {
@@ -697,7 +704,7 @@ describe('data table', () => {
         By.directive(ContentCellComponent)
       );
 
-      expect(cells.length).toBe(3);
+      expect(cells.length).toBe(4);
 
       cells.forEach((cell) => {
         cell.nativeElement.dispatchEvent(new MouseEvent('contextmenu'));
@@ -772,7 +779,7 @@ describe('data table', () => {
         By.directive(ContentCellComponent)
       );
 
-      expect(cells.length).toBe(3);
+      expect(cells.length).toBe(4);
 
       cells.forEach((cell) => {
         cell.nativeElement.dispatchEvent(new MouseEvent('contextmenu'));
@@ -796,5 +803,69 @@ describe('data table', () => {
         }
       });
     });
+
+    it('renders an upwards arrow when the sort direction is ascending', () => {
+      const fixture = createComponent({
+        headers: mockHeaders,
+        data: mockTableData,
+        potentialColumns: mockPotentialColumns,
+        sortingInfo: {
+          name: 'run',
+          order: SortingOrder.ASCENDING,
+        },
+      });
+      const header = fixture.debugElement.query(
+        By.directive(HeaderCellComponent)
+      );
+      header.nativeElement.dispatchEvent(new MouseEvent('contextmenu'));
+      fixture.detectChanges();
+
+      expect(
+        fixture.debugElement
+          .query(By.css('.context-menu-button.sort-button mat-icon'))
+          .nativeElement.getAttribute('svgIcon')
+      ).toBe('arrow_downward_24px');
+    });
+
+    it('renders a downwards arrow when the sort direction is descending', () => {
+      const fixture = createComponent({
+        headers: mockHeaders,
+        data: mockTableData,
+        potentialColumns: mockPotentialColumns,
+        sortingInfo: {
+          name: 'run',
+          order: SortingOrder.DESCENDING,
+        },
+      });
+      const header = fixture.debugElement.query(
+        By.directive(HeaderCellComponent)
+      );
+      header.nativeElement.dispatchEvent(new MouseEvent('contextmenu'));
+      fixture.detectChanges();
+
+      expect(
+        fixture.debugElement
+          .query(By.css('.context-menu-button.sort-button mat-icon'))
+          .nativeElement.getAttribute('svgIcon')
+      ).toBe('arrow_upward_24px');
+    });
+
+    it('displays a message when empty', () => {
+      const fixture = createComponent({
+        headers: mockHeaders,
+        data: mockTableData,
+        potentialColumns: mockPotentialColumns,
+      });
+      const fixedHeader = fixture.debugElement.queryAll(
+        By.directive(HeaderCellComponent)
+      )[4];
+      fixedHeader.nativeElement.dispatchEvent(new MouseEvent('contextmenu'));
+      fixture.detectChanges();
+
+      const contextMenu = fixture.debugElement.query(By.css('.context-menu'));
+      expect(
+        contextMenu.nativeElement.innerHTML.includes('No Actions Available')
+      ).toBeTrue();
+    });
   });
 });

tensorboard/webapp/widgets/data_table/header_cell_component.ng.html

@@ -22,20 +22,6 @@
   [ngClass]="highlightStyle$ | async"
 >
   <ng-content></ng-content>
-  <div
-    *ngIf="hparamsEnabled && header.removable"
-    class="delete-button-container"
-  >
-    <button
-      class="delete-button"
-      mat-icon-button
-      i18n-aria-label="A button to delete a data table column."
-      aria-label="Delete column"
-      (click)="deleteButtonClicked.emit(header)"
-    >
-      <mat-icon svgIcon="close_24px"> </mat-icon>
-    </button>
-  </div>
   <tb-data-table-header [header]="header"></tb-data-table-header>
   <div *ngIf="header.sortable" class="sorting-icon-container">
     <mat-icon
@@ -60,15 +46,12 @@
     ></mat-icon>
   </div>
   <div
-    *ngIf="header.removable || header.sortable"
-    class="context-menu-container"
+    *ngIf="(header.removable || header.sortable) && !disableContextMenu"
+    class="context-menu-container show-on-hover"
   >
-    <button
-      mat-icon-button
-      class="context-menu-trigger"
+    <mat-icon
       (click)="onContextMenuOpened($event)"
-    >
-      <mat-icon svgIcon="more_vert_24px"></mat-icon>
-    </button>
+      svgIcon="more_vert_24px"
+    ></mat-icon>
   </div>
 </div>

tensorboard/webapp/widgets/data_table/header_cell_component.scss

@@ -25,17 +25,20 @@ $_icon_size: 12px;
   vertical-align: bottom;
 
   &:hover {
-    .context-menu-container {
-      opacity: 0.8;
-    }
-
     .show-on-hover {
       opacity: 0.3;
     }
   }
+
+  .show-on-hover {
+    &:hover {
+      opacity: 1;
+    }
+  }
 }
 
-.sorting-icon-container {
+.sorting-icon-container,
+.context-menu-container {
   width: $_icon_size;
   height: $_icon_size;
   border-radius: 5px;
@@ -45,12 +48,6 @@ $_icon_size: 12px;
   align-items: center;
   display: flex;
 
-  &:hover {
-    .delete-button-container {
-      opacity: 1;
-    }
-  }
-
   mat-icon {
     height: $_icon_size;
     width: $_icon_size;
@@ -63,30 +60,6 @@ $_icon_size: 12px;
   }
 }
 
-.delete-button {
-  display: flex;
-  align-items: center;
-  color: #fff;
-  font-size: $_icon_size;
-  height: $_icon_size;
-  width: $_icon_size;
-  line-height: $_icon_size;
-  cursor: pointer;
-
-  .mat-icon {
-    line-height: $_icon_size;
-  }
-}
-
-.delete-button-container {
-  border-radius: 50%;
-  background-color: mat.get-color-from-palette(mat.$gray-palette, 400);
-  margin-left: -$_icon_size;
-  height: $_icon_size;
-  opacity: 0;
-  position: absolute;
-}
-
 .show {
   opacity: 1;
 }
@@ -106,15 +79,3 @@ $_icon_size: 12px;
 .highlight-border-left {
   border-left: 2px solid mat.get-color-from-palette($_accent);
 }
-
-.context-menu-container {
-  opacity: 0;
-
-  .context-menu-trigger {
-    width: 12px;
-
-    mat-icon {
-      height: unset;
-    }
-  }
-}

tensorboard/webapp/widgets/data_table/header_cell_component.ts

@@ -21,12 +21,7 @@ import {
   Input,
   Output,
 } from '@angular/core';
-import {
-  ColumnHeader,
-  ColumnHeaderType,
-  SortingInfo,
-  SortingOrder,
-} from './types';
+import {ColumnHeader, SortingInfo, SortingOrder} from './types';
 import {BehaviorSubject} from 'rxjs';
 
 @Component({
@@ -39,14 +34,12 @@ export class HeaderCellComponent {
   @Input() header!: ColumnHeader;
   @Input() sortingInfo!: SortingInfo;
   @Input() hparamsEnabled?: boolean = false;
+  @Input() disableContextMenu?: boolean = false;
 
   @Output() dragStart = new EventEmitter<ColumnHeader>();
   @Output() dragEnd = new EventEmitter<void>();
   @Output() dragEnter = new EventEmitter<ColumnHeader>();
   @Output() headerClicked = new EventEmitter<string>();
-  @Output() deleteButtonClicked = new EventEmitter<{
-    headerType: ColumnHeaderType;
-  }>();
   @Output() contextMenuOpened = new EventEmitter<MouseEvent>();
 
   highlightStyle$: BehaviorSubject<Object> = new BehaviorSubject<Object>({});
@@ -55,6 +48,9 @@ export class HeaderCellComponent {
 
   @HostListener('contextmenu', ['$event'])
   onContextMenuOpened(event: MouseEvent) {
+    if (this.disableContextMenu) {
+      return;
+    }
     this.contextMenuOpened.emit(event);
   }