From b98cecbf7373675cb8bbba599abf1218e95bea56 Mon Sep 17 00:00:00 2001
From: jameshollyer <j.c.hollyer@gmail.com>
Date: Thu, 18 May 2023 21:24:16 +0000
Subject: [PATCH 1/3] Data Table: refactor data table to use divs instead of
 html table

---
 .../data_table/data_table_component.ng.html   | 125 +++++++++---------
 .../data_table/data_table_component.scss      |  46 ++++---
 .../widgets/data_table/data_table_test.ts     |  44 ++++--
 3 files changed, 117 insertions(+), 98 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 b50a93b7b8..24af6ed59b 100644
--- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html
+++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html
@@ -12,74 +12,67 @@
 limitations under the License.
 -->
 
-<div>
-  <table class="data-table">
-    <thead>
-      <tr>
-        <th>
-          <!-- This header is intentionally left blank for the color column -->
-        </th>
-        <ng-container *ngFor="let header of headers;">
-          <th *ngIf="showColumn(header)" (click)="headerClicked(header.name)">
-            <div
-              [draggable]="columnCustomizationEnabled"
-              (dragstart)="dragStart(header)"
-              (dragend)="dragEnd()"
-              (dragenter)="dragEnter(header)"
-              class="cell"
-              [ngClass]="getHeaderHighlightStyle(header.name)"
-            >
-              <tb-data-table-header [header]="header"></tb-data-table-header>
+<div class="data-table">
+  <header>
+    <div class="col"></div>
+    <ng-container *ngFor="let header of headers;">
+      <div
+        class="col"
+        *ngIf="showColumn(header)"
+        (click)="headerClicked(header.name)"
+      >
+        <div
+          [draggable]="columnCustomizationEnabled"
+          (dragstart)="dragStart(header)"
+          (dragend)="dragEnd()"
+          (dragenter)="dragEnter(header)"
+          class="cell"
+          [ngClass]="getHeaderHighlightStyle(header.name)"
+        >
+          <tb-data-table-header [header]="header"></tb-data-table-header>
 
-              <div class="sorting-icon-container">
-                <mat-icon
-                  *ngIf="sortingInfo.order === SortingOrder.ASCENDING || header.name !== sortingInfo.name"
-                  [ngClass]="header.name === sortingInfo.name ? 'show' : 'show-on-hover'"
-                  svgIcon="arrow_upward_24px"
-                ></mat-icon>
-                <mat-icon
-                  *ngIf="sortingInfo.order === SortingOrder.DESCENDING && header.name === sortingInfo.name"
-                  [ngClass]="header.name === sortingInfo.name ? 'show' : 'show-on-hover'"
-                  svgIcon="arrow_downward_24px"
-                ></mat-icon>
-              </div>
-            </div>
-          </th>
-        </ng-container>
-      </tr>
-    </thead>
-    <tbody>
-      <ng-container *ngFor="let runData of data;">
-        <tr class="row">
-          <td class="row-circle">
-            <span [style.backgroundColor]="runData['color']"></span>
-          </td>
-          <ng-container *ngFor="let header of headers;">
-            <td *ngIf="showColumn(header)" [ngSwitch]="header.type">
-              <div *ngSwitchCase="ColumnHeaders.VALUE_CHANGE" class="cell">
-                <ng-container
-                  *ngTemplateOutlet="arrow; context: {$implicit:runData[header.name]}"
-                ></ng-container>
-                {{ getFormattedDataForColumn(header.type, runData[header.name])
-                }}
-              </div>
-              <div *ngSwitchCase="ColumnHeaders.PERCENTAGE_CHANGE" class="cell">
-                <ng-container
-                  *ngTemplateOutlet="arrow; context: {$implicit:runData[header.name]}"
-                ></ng-container>
-                {{ getFormattedDataForColumn(header.type, runData[header.name])
-                }}
-              </div>
-              <div *ngSwitchDefault class="cell extra-right-padding">
-                {{ getFormattedDataForColumn(header.type, runData[header.name])
-                }}
-              </div>
-            </td>
-          </ng-container>
-        </tr>
+          <div class="sorting-icon-container">
+            <mat-icon
+              *ngIf="sortingInfo.order === SortingOrder.ASCENDING || header.name !== sortingInfo.name"
+              [ngClass]="header.name === sortingInfo.name ? 'show' : 'show-on-hover'"
+              svgIcon="arrow_upward_24px"
+            ></mat-icon>
+            <mat-icon
+              *ngIf="sortingInfo.order === SortingOrder.DESCENDING && header.name === sortingInfo.name"
+              [ngClass]="header.name === sortingInfo.name ? 'show' : 'show-on-hover'"
+              svgIcon="arrow_downward_24px"
+            ></mat-icon>
+          </div>
+        </div>
+      </div>
+    </ng-container>
+  </header>
+  <ng-container *ngFor="let runData of data;">
+    <div class="row">
+      <div class="col row-circle">
+        <span [style.backgroundColor]="runData['color']"></span>
+      </div>
+      <ng-container *ngFor="let header of headers;">
+        <div class="col" *ngIf="showColumn(header)" [ngSwitch]="header.type">
+          <div *ngSwitchCase="ColumnHeaders.VALUE_CHANGE" class="cell">
+            <ng-container
+              *ngTemplateOutlet="arrow; context: {$implicit:runData[header.name]}"
+            ></ng-container>
+            {{ getFormattedDataForColumn(header.type, runData[header.name]) }}
+          </div>
+          <div *ngSwitchCase="ColumnHeaders.PERCENTAGE_CHANGE" class="cell">
+            <ng-container
+              *ngTemplateOutlet="arrow; context: {$implicit:runData[header.name]}"
+            ></ng-container>
+            {{ getFormattedDataForColumn(header.type, runData[header.name]) }}
+          </div>
+          <div *ngSwitchDefault class="cell extra-right-padding">
+            {{ getFormattedDataForColumn(header.type, runData[header.name]) }}
+          </div>
+        </div>
       </ng-container>
-    </tbody>
-  </table>
+    </div>
+  </ng-container>
 </div>
 <ng-template #arrow let-value>
   <mat-icon *ngIf="value >= 0" svgIcon="arrow_upward_24px"></mat-icon>
diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.scss b/tensorboard/webapp/widgets/data_table/data_table_component.scss
index b848f4bf9a..03a66cde5c 100644
--- a/tensorboard/webapp/widgets/data_table/data_table_component.scss
+++ b/tensorboard/webapp/widgets/data_table/data_table_component.scss
@@ -20,13 +20,24 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
 .data-table {
   border-spacing: 4px;
   font-size: 13px;
+  display: table;
   width: 100%;
 
-  th {
+  header,
+  .row {
+    display: table-row;
+  }
+
+  .col {
+    display: table-cell;
+  }
+
+  header {
     background-color: mat.get-color-from-palette($tb-background, background);
     position: sticky;
     text-align: left;
     top: 0;
+    font-weight: bold;
     vertical-align: bottom;
 
     &:hover {
@@ -36,11 +47,17 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
     @include tb-dark-theme {
       background-color: map-get($tb-dark-background, background);
     }
+    .col:hover .show-on-hover {
+      opacity: 0.3;
+    }
+
+    .col {
+      vertical-align: bottom;
+    }
   }
 
-  .cell {
-    align-items: center;
-    display: flex;
+  .col {
+    padding: 1px;
   }
 
   .extra-right-padding {
@@ -48,19 +65,8 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
     padding-right: 1px;
   }
 
-  .row {
-    white-space: nowrap;
-  }
-
   $_circle-size: 12px;
 
-  .row-circle {
-    align-items: center;
-    display: inline-flex;
-    height: $_circle-size;
-    width: $_circle-size;
-  }
-
   .row-circle > span {
     border-radius: 50%;
     border: 1px solid rgba(255, 255, 255, 0.4);
@@ -68,6 +74,12 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
     // Subtract by border width (1px on both sides)
     height: $_circle-size - 2px;
     width: $_circle-size - 2px;
+    vertical-align: middle;
+  }
+
+  .cell {
+    align-items: center;
+    display: flex;
   }
 
   .cell mat-icon {
@@ -89,10 +101,6 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
     opacity: 0;
   }
 
-  th:hover .show-on-hover {
-    opacity: 0.3;
-  }
-
   .highlight {
     background-color: mat.get-color-from-palette(mat.$gray-palette, 200);
   }
diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts
index 7ae8a9b3f2..5afc642169 100644
--- a/tensorboard/webapp/widgets/data_table/data_table_test.ts
+++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts
@@ -135,7 +135,9 @@ describe('data table', () => {
       ],
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
 
     // The first header should always be blank as it is the run color column.
     expect(headerElements[0].nativeElement.innerText).toBe('');
@@ -258,7 +260,7 @@ describe('data table', () => {
       ],
     });
     fixture.detectChanges();
-    const dataElements = fixture.debugElement.queryAll(By.css('td'));
+    const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
 
     // The first header should always be blank as it is the run color column.
     expect(dataElements[0].nativeElement.innerText).toBe('');
@@ -321,8 +323,10 @@ describe('data table', () => {
       ],
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
-    const dataElements = fixture.debugElement.queryAll(By.css('td'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
+    const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
 
     // The first header should always be blank as it is the run color column.
     expect(headerElements[0].nativeElement.innerText).toBe('');
@@ -366,7 +370,7 @@ describe('data table', () => {
       data: [{id: 'someid'}],
     });
     fixture.detectChanges();
-    const dataElements = fixture.debugElement.queryAll(By.css('td'));
+    const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
 
     // The first header should always be blank as it is the run color column.
     expect(dataElements[0].nativeElement.innerText).toBe('');
@@ -406,7 +410,9 @@ describe('data table', () => {
       ],
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
 
     headerElements[3].triggerEventHandler('click', {});
     expect(sortDataBySpy).toHaveBeenCalledOnceWith({
@@ -451,7 +457,9 @@ describe('data table', () => {
       },
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
 
     headerElements[3].triggerEventHandler('click', {});
     expect(sortDataBySpy).toHaveBeenCalledOnceWith({
@@ -490,7 +498,9 @@ describe('data table', () => {
       },
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
 
     expect(
       headerElements[1]
@@ -553,7 +563,9 @@ describe('data table', () => {
       },
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
 
     expect(
       headerElements[1]
@@ -616,7 +628,9 @@ describe('data table', () => {
       },
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
 
     headerElements[2].query(By.css('.cell')).triggerEventHandler('dragstart');
     headerElements[1].query(By.css('.cell')).triggerEventHandler('dragenter');
@@ -684,7 +698,9 @@ describe('data table', () => {
       },
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
 
     headerElements[2].query(By.css('.cell')).triggerEventHandler('dragstart');
     headerElements[3].query(By.css('.cell')).triggerEventHandler('dragenter');
@@ -763,8 +779,10 @@ describe('data table', () => {
       smoothingEnabled: false,
     });
     fixture.detectChanges();
-    const headerElements = fixture.debugElement.queryAll(By.css('th'));
-    const dataElements = fixture.debugElement.queryAll(By.css('td'));
+    const headerElements = fixture.debugElement.queryAll(
+      By.css('header > .col')
+    );
+    const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
 
     // The first header should always be blank as it is the run color column.
     expect(headerElements[0].nativeElement.innerText).toBe('');

From 6a208b8f564ba484aae31f7dd59e3bd42ca54b0b Mon Sep 17 00:00:00 2001
From: jameshollyer <j.c.hollyer@gmail.com>
Date: Fri, 19 May 2023 16:55:10 +0000
Subject: [PATCH 2/3] replace header element with a div that has header as a
 class

---
 .../webapp/widgets/data_table/data_table_component.ng.html    | 4 ++--
 .../webapp/widgets/data_table/data_table_component.scss       | 4 ++--
 2 files changed, 4 insertions(+), 4 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 24af6ed59b..9f3eaa6f93 100644
--- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html
+++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html
@@ -13,7 +13,7 @@
 -->
 
 <div class="data-table">
-  <header>
+  <div class="header">
     <div class="col"></div>
     <ng-container *ngFor="let header of headers;">
       <div
@@ -46,7 +46,7 @@
         </div>
       </div>
     </ng-container>
-  </header>
+  </div>
   <ng-container *ngFor="let runData of data;">
     <div class="row">
       <div class="col row-circle">
diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.scss b/tensorboard/webapp/widgets/data_table/data_table_component.scss
index 03a66cde5c..79b97a2b30 100644
--- a/tensorboard/webapp/widgets/data_table/data_table_component.scss
+++ b/tensorboard/webapp/widgets/data_table/data_table_component.scss
@@ -23,7 +23,7 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
   display: table;
   width: 100%;
 
-  header,
+  .header,
   .row {
     display: table-row;
   }
@@ -32,7 +32,7 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
     display: table-cell;
   }
 
-  header {
+  .header {
     background-color: mat.get-color-from-palette($tb-background, background);
     position: sticky;
     text-align: left;

From 551a537fbb18dc6bacba90a5d35b3c0812d7667a Mon Sep 17 00:00:00 2001
From: jameshollyer <j.c.hollyer@gmail.com>
Date: Fri, 19 May 2023 17:24:09 +0000
Subject: [PATCH 3/3] fix tests to use new header element

---
 .../widgets/data_table/data_table_test.ts      | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts
index 5afc642169..2d7ea22e1b 100644
--- a/tensorboard/webapp/widgets/data_table/data_table_test.ts
+++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts
@@ -136,7 +136,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
 
     // The first header should always be blank as it is the run color column.
@@ -324,7 +324,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
     const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
 
@@ -411,7 +411,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
 
     headerElements[3].triggerEventHandler('click', {});
@@ -458,7 +458,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
 
     headerElements[3].triggerEventHandler('click', {});
@@ -499,7 +499,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
 
     expect(
@@ -564,7 +564,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
 
     expect(
@@ -629,7 +629,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
 
     headerElements[2].query(By.css('.cell')).triggerEventHandler('dragstart');
@@ -699,7 +699,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
 
     headerElements[2].query(By.css('.cell')).triggerEventHandler('dragstart');
@@ -780,7 +780,7 @@ describe('data table', () => {
     });
     fixture.detectChanges();
     const headerElements = fixture.debugElement.queryAll(
-      By.css('header > .col')
+      By.css('.header > .col')
     );
     const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));