Skip to content

Commit 200ba5e

Browse files
authored
Bug Fix: Fix color picker regression (#6682)
## Motivation for features / changes There was a regression associated with the new runs table which meant that changing the color of a run immediately closed the color picker. ## Technical description of changes Previously the runs table did not have a dependency on the metrics state. Now that it does, changing the color of a run leads to a state change which then in turn rerenders the row and thus closed the color picker. The state dependency is important so that cannot be undone, however, we don't actually need to re render the rows when the color changes so I've update the render logic to ignore the `color` attribute (I know it's a little hacky). ## Screenshots of UI changes (or N/A) Before: ![45503b0a-11af-4660-b168-fb54b6262a00](https://github.com/tensorflow/tensorboard/assets/78179109/f08dd518-1b0e-4287-b933-87c1d4f374d1) After - With Paint Flashing ![9c5d3e86-9369-4c37-87ab-66bb74662c07](https://github.com/tensorflow/tensorboard/assets/78179109/26176959-c196-4d5b-923d-8686c4264dce) After - Without Paint Flashing ![57df4fe5-536a-49f0-aeda-16028912e35b](https://github.com/tensorflow/tensorboard/assets/78179109/bd098ea7-4cac-4b01-961d-ee7203f5a7f2) ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Open the color picker 3) Change the color 4) Observe the color picker does not close 5) Check a row and observe that it is rerendered. ## Alternate designs / implementations considered (or N/A)
1 parent dc05adf commit 200ba5e

File tree

3 files changed

+30
-1
lines changed

3 files changed

+30
-1
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@
6464
></ng-container>
6565

6666
<ng-container content>
67-
<ng-container *ngFor="let dataRow of data">
67+
<ng-container *ngFor="let dataRow of data; trackBy: trackByRuns">
6868
<tb-data-table-content-row [attr.data-id]="dataRow.id">
6969
<ng-container *ngFor="let header of getHeaders()">
7070
<tb-data-table-content-cell

tensorboard/webapp/runs/views/runs_table/runs_data_table.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,4 +127,16 @@ export class RunsDataTable {
127127
const input = event.target! as HTMLInputElement;
128128
this.onRegexFilterChange.emit(input.value);
129129
}
130+
131+
/**
132+
* Using the `trackBy` directive allows you to control when an element contained
133+
* by an `ngFor` is rerendered. In this case it is important that changes to
134+
* the `color` attribute do NOT trigger rerenders because doing so will recreate
135+
* and close the colorPicker.
136+
*/
137+
trackByRuns(index: number, data: TableData) {
138+
const dataWithoutColor = {...data};
139+
delete dataWithoutColor.color;
140+
return JSON.stringify(dataWithoutColor);
141+
}
130142
}

tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,4 +355,21 @@ describe('runs_data_table', () => {
355355

356356
expect(onRegexFilterChangeSpy).toHaveBeenCalledWith('myRegex');
357357
});
358+
359+
it('trackByRuns serializes data while ignoring color', () => {
360+
const fixture = createComponent({});
361+
const dataTable = fixture.debugElement.query(By.directive(RunsDataTable));
362+
expect(
363+
dataTable.componentInstance.trackByRuns(0, {
364+
id: 'run1',
365+
color: 'orange',
366+
hparam1: 1.234,
367+
})
368+
).toEqual(
369+
JSON.stringify({
370+
id: 'run1',
371+
hparam1: 1.234,
372+
})
373+
);
374+
});
358375
});

0 commit comments

Comments
 (0)