Skip to content

Commit d333cf5

Browse files
JamesHollyerJamesHollyer
authored
Data Table: refactor data table to use divs instead of html table (#6397)
## Motivation for features / changes This probably should have been done when the data table was created. html table is generally considered old technology. Building tables with divs is preferred as it allows for more flexibility. ## Technical description of changes Pretty straight forward swap to using display table, table-row, table-column instead of table, tr, td. For the header we use a \<header\> to wrap it and still use the .col. Because of this header specific logic for .col was moved inside the header object. I also had to mess around with the row-circle class to get the color circle to align properly because the display is now table-cell instead of flex. ## Screenshots of UI changes (or N/A) New table looks like this. <img width="731" alt="Screenshot 2023-05-18 at 4 41 01 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/4e7eadc2-18c7-4320-919e-9d30573d72a5"> The only change is that the circle icon moves ever so slightly(to see the difference more clearly googlers can check the screenshot diffs here cl/529831620). ## Detailed steps to verify changes work correctly (as executed by you) A couple CSS queries changes and all the tests pass. I also clicked around and tested functionality in local instance. ## Alternate designs / implementations considered (or N/A) Considered leaving it as is.
1 parent a0865d9 commit d333cf5

File tree

3 files changed

+117
-98
lines changed

3 files changed

+117
-98
lines changed

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

Lines changed: 59 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -12,74 +12,67 @@
1212
limitations under the License.
1313
-->
1414

15-
<div>
16-
<table class="data-table">
17-
<thead>
18-
<tr>
19-
<th>
20-
<!-- This header is intentionally left blank for the color column -->
21-
</th>
22-
<ng-container *ngFor="let header of headers;">
23-
<th *ngIf="showColumn(header)" (click)="headerClicked(header.name)">
24-
<div
25-
[draggable]="columnCustomizationEnabled"
26-
(dragstart)="dragStart(header)"
27-
(dragend)="dragEnd()"
28-
(dragenter)="dragEnter(header)"
29-
class="cell"
30-
[ngClass]="getHeaderHighlightStyle(header.name)"
31-
>
32-
<tb-data-table-header [header]="header"></tb-data-table-header>
15+
<div class="data-table">
16+
<div class="header">
17+
<div class="col"></div>
18+
<ng-container *ngFor="let header of headers;">
19+
<div
20+
class="col"
21+
*ngIf="showColumn(header)"
22+
(click)="headerClicked(header.name)"
23+
>
24+
<div
25+
[draggable]="columnCustomizationEnabled"
26+
(dragstart)="dragStart(header)"
27+
(dragend)="dragEnd()"
28+
(dragenter)="dragEnter(header)"
29+
class="cell"
30+
[ngClass]="getHeaderHighlightStyle(header.name)"
31+
>
32+
<tb-data-table-header [header]="header"></tb-data-table-header>
3333

34-
<div class="sorting-icon-container">
35-
<mat-icon
36-
*ngIf="sortingInfo.order === SortingOrder.ASCENDING || header.name !== sortingInfo.name"
37-
[ngClass]="header.name === sortingInfo.name ? 'show' : 'show-on-hover'"
38-
svgIcon="arrow_upward_24px"
39-
></mat-icon>
40-
<mat-icon
41-
*ngIf="sortingInfo.order === SortingOrder.DESCENDING && header.name === sortingInfo.name"
42-
[ngClass]="header.name === sortingInfo.name ? 'show' : 'show-on-hover'"
43-
svgIcon="arrow_downward_24px"
44-
></mat-icon>
45-
</div>
46-
</div>
47-
</th>
48-
</ng-container>
49-
</tr>
50-
</thead>
51-
<tbody>
52-
<ng-container *ngFor="let runData of data;">
53-
<tr class="row">
54-
<td class="row-circle">
55-
<span [style.backgroundColor]="runData['color']"></span>
56-
</td>
57-
<ng-container *ngFor="let header of headers;">
58-
<td *ngIf="showColumn(header)" [ngSwitch]="header.type">
59-
<div *ngSwitchCase="ColumnHeaders.VALUE_CHANGE" class="cell">
60-
<ng-container
61-
*ngTemplateOutlet="arrow; context: {$implicit:runData[header.name]}"
62-
></ng-container>
63-
{{ getFormattedDataForColumn(header.type, runData[header.name])
64-
}}
65-
</div>
66-
<div *ngSwitchCase="ColumnHeaders.PERCENTAGE_CHANGE" class="cell">
67-
<ng-container
68-
*ngTemplateOutlet="arrow; context: {$implicit:runData[header.name]}"
69-
></ng-container>
70-
{{ getFormattedDataForColumn(header.type, runData[header.name])
71-
}}
72-
</div>
73-
<div *ngSwitchDefault class="cell extra-right-padding">
74-
{{ getFormattedDataForColumn(header.type, runData[header.name])
75-
}}
76-
</div>
77-
</td>
78-
</ng-container>
79-
</tr>
34+
<div class="sorting-icon-container">
35+
<mat-icon
36+
*ngIf="sortingInfo.order === SortingOrder.ASCENDING || header.name !== sortingInfo.name"
37+
[ngClass]="header.name === sortingInfo.name ? 'show' : 'show-on-hover'"
38+
svgIcon="arrow_upward_24px"
39+
></mat-icon>
40+
<mat-icon
41+
*ngIf="sortingInfo.order === SortingOrder.DESCENDING && header.name === sortingInfo.name"
42+
[ngClass]="header.name === sortingInfo.name ? 'show' : 'show-on-hover'"
43+
svgIcon="arrow_downward_24px"
44+
></mat-icon>
45+
</div>
46+
</div>
47+
</div>
48+
</ng-container>
49+
</div>
50+
<ng-container *ngFor="let runData of data;">
51+
<div class="row">
52+
<div class="col row-circle">
53+
<span [style.backgroundColor]="runData['color']"></span>
54+
</div>
55+
<ng-container *ngFor="let header of headers;">
56+
<div class="col" *ngIf="showColumn(header)" [ngSwitch]="header.type">
57+
<div *ngSwitchCase="ColumnHeaders.VALUE_CHANGE" class="cell">
58+
<ng-container
59+
*ngTemplateOutlet="arrow; context: {$implicit:runData[header.name]}"
60+
></ng-container>
61+
{{ getFormattedDataForColumn(header.type, runData[header.name]) }}
62+
</div>
63+
<div *ngSwitchCase="ColumnHeaders.PERCENTAGE_CHANGE" class="cell">
64+
<ng-container
65+
*ngTemplateOutlet="arrow; context: {$implicit:runData[header.name]}"
66+
></ng-container>
67+
{{ getFormattedDataForColumn(header.type, runData[header.name]) }}
68+
</div>
69+
<div *ngSwitchDefault class="cell extra-right-padding">
70+
{{ getFormattedDataForColumn(header.type, runData[header.name]) }}
71+
</div>
72+
</div>
8073
</ng-container>
81-
</tbody>
82-
</table>
74+
</div>
75+
</ng-container>
8376
</div>
8477
<ng-template #arrow let-value>
8578
<mat-icon *ngIf="value >= 0" svgIcon="arrow_upward_24px"></mat-icon>

‎tensorboard/webapp/widgets/data_table/data_table_component.scss‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,24 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
2020
.data-table {
2121
border-spacing: 4px;
2222
font-size: 13px;
23+
display: table;
2324
width: 100%;
2425

25-
th {
26+
.header,
27+
.row {
28+
display: table-row;
29+
}
30+
31+
.col {
32+
display: table-cell;
33+
}
34+
35+
.header {
2636
background-color: mat.get-color-from-palette($tb-background, background);
2737
position: sticky;
2838
text-align: left;
2939
top: 0;
40+
font-weight: bold;
3041
vertical-align: bottom;
3142

3243
&:hover {
@@ -36,38 +47,39 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
3647
@include tb-dark-theme {
3748
background-color: map-get($tb-dark-background, background);
3849
}
50+
.col:hover .show-on-hover {
51+
opacity: 0.3;
52+
}
53+
54+
.col {
55+
vertical-align: bottom;
56+
}
3957
}
4058

41-
.cell {
42-
align-items: center;
43-
display: flex;
59+
.col {
60+
padding: 1px;
4461
}
4562

4663
.extra-right-padding {
4764
// Add artificial padding to keep consistent with icons which have whitespace
4865
padding-right: 1px;
4966
}
5067

51-
.row {
52-
white-space: nowrap;
53-
}
54-
5568
$_circle-size: 12px;
5669

57-
.row-circle {
58-
align-items: center;
59-
display: inline-flex;
60-
height: $_circle-size;
61-
width: $_circle-size;
62-
}
63-
6470
.row-circle > span {
6571
border-radius: 50%;
6672
border: 1px solid rgba(255, 255, 255, 0.4);
6773
display: inline-block;
6874
// Subtract by border width (1px on both sides)
6975
height: $_circle-size - 2px;
7076
width: $_circle-size - 2px;
77+
vertical-align: middle;
78+
}
79+
80+
.cell {
81+
align-items: center;
82+
display: flex;
7183
}
7284

7385
.cell mat-icon {
@@ -89,10 +101,6 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent);
89101
opacity: 0;
90102
}
91103

92-
th:hover .show-on-hover {
93-
opacity: 0.3;
94-
}
95-
96104
.highlight {
97105
background-color: mat.get-color-from-palette(mat.$gray-palette, 200);
98106
}

‎tensorboard/webapp/widgets/data_table/data_table_test.ts‎

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,9 @@ describe('data table', () => {
135135
],
136136
});
137137
fixture.detectChanges();
138-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
138+
const headerElements = fixture.debugElement.queryAll(
139+
By.css('.header > .col')
140+
);
139141

140142
// The first header should always be blank as it is the run color column.
141143
expect(headerElements[0].nativeElement.innerText).toBe('');
@@ -258,7 +260,7 @@ describe('data table', () => {
258260
],
259261
});
260262
fixture.detectChanges();
261-
const dataElements = fixture.debugElement.queryAll(By.css('td'));
263+
const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
262264

263265
// The first header should always be blank as it is the run color column.
264266
expect(dataElements[0].nativeElement.innerText).toBe('');
@@ -321,8 +323,10 @@ describe('data table', () => {
321323
],
322324
});
323325
fixture.detectChanges();
324-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
325-
const dataElements = fixture.debugElement.queryAll(By.css('td'));
326+
const headerElements = fixture.debugElement.queryAll(
327+
By.css('.header > .col')
328+
);
329+
const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
326330

327331
// The first header should always be blank as it is the run color column.
328332
expect(headerElements[0].nativeElement.innerText).toBe('');
@@ -366,7 +370,7 @@ describe('data table', () => {
366370
data: [{id: 'someid'}],
367371
});
368372
fixture.detectChanges();
369-
const dataElements = fixture.debugElement.queryAll(By.css('td'));
373+
const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
370374

371375
// The first header should always be blank as it is the run color column.
372376
expect(dataElements[0].nativeElement.innerText).toBe('');
@@ -406,7 +410,9 @@ describe('data table', () => {
406410
],
407411
});
408412
fixture.detectChanges();
409-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
413+
const headerElements = fixture.debugElement.queryAll(
414+
By.css('.header > .col')
415+
);
410416

411417
headerElements[3].triggerEventHandler('click', {});
412418
expect(sortDataBySpy).toHaveBeenCalledOnceWith({
@@ -451,7 +457,9 @@ describe('data table', () => {
451457
},
452458
});
453459
fixture.detectChanges();
454-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
460+
const headerElements = fixture.debugElement.queryAll(
461+
By.css('.header > .col')
462+
);
455463

456464
headerElements[3].triggerEventHandler('click', {});
457465
expect(sortDataBySpy).toHaveBeenCalledOnceWith({
@@ -490,7 +498,9 @@ describe('data table', () => {
490498
},
491499
});
492500
fixture.detectChanges();
493-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
501+
const headerElements = fixture.debugElement.queryAll(
502+
By.css('.header > .col')
503+
);
494504

495505
expect(
496506
headerElements[1]
@@ -553,7 +563,9 @@ describe('data table', () => {
553563
},
554564
});
555565
fixture.detectChanges();
556-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
566+
const headerElements = fixture.debugElement.queryAll(
567+
By.css('.header > .col')
568+
);
557569

558570
expect(
559571
headerElements[1]
@@ -616,7 +628,9 @@ describe('data table', () => {
616628
},
617629
});
618630
fixture.detectChanges();
619-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
631+
const headerElements = fixture.debugElement.queryAll(
632+
By.css('.header > .col')
633+
);
620634

621635
headerElements[2].query(By.css('.cell')).triggerEventHandler('dragstart');
622636
headerElements[1].query(By.css('.cell')).triggerEventHandler('dragenter');
@@ -684,7 +698,9 @@ describe('data table', () => {
684698
},
685699
});
686700
fixture.detectChanges();
687-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
701+
const headerElements = fixture.debugElement.queryAll(
702+
By.css('.header > .col')
703+
);
688704

689705
headerElements[2].query(By.css('.cell')).triggerEventHandler('dragstart');
690706
headerElements[3].query(By.css('.cell')).triggerEventHandler('dragenter');
@@ -763,8 +779,10 @@ describe('data table', () => {
763779
smoothingEnabled: false,
764780
});
765781
fixture.detectChanges();
766-
const headerElements = fixture.debugElement.queryAll(By.css('th'));
767-
const dataElements = fixture.debugElement.queryAll(By.css('td'));
782+
const headerElements = fixture.debugElement.queryAll(
783+
By.css('.header > .col')
784+
);
785+
const dataElements = fixture.debugElement.queryAll(By.css('.row > .col'));
768786

769787
// The first header should always be blank as it is the run color column.
770788
expect(headerElements[0].nativeElement.innerText).toBe('');

0 commit comments

Comments
 (0)