Skip to content

Commit

Permalink
Bug Fix: Fix color picker regression (#6682)
Browse files Browse the repository at this point in the history
## 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)
  • Loading branch information
rileyajones committed Nov 14, 2023
1 parent dc05adf commit 200ba5e
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 1 deletion.
Expand Up @@ -64,7 +64,7 @@
></ng-container>

<ng-container content>
<ng-container *ngFor="let dataRow of data">
<ng-container *ngFor="let dataRow of data; trackBy: trackByRuns">
<tb-data-table-content-row [attr.data-id]="dataRow.id">
<ng-container *ngFor="let header of getHeaders()">
<tb-data-table-content-cell
Expand Down
12 changes: 12 additions & 0 deletions tensorboard/webapp/runs/views/runs_table/runs_data_table.ts
Expand Up @@ -127,4 +127,16 @@ export class RunsDataTable {
const input = event.target! as HTMLInputElement;
this.onRegexFilterChange.emit(input.value);
}

/**
* Using the `trackBy` directive allows you to control when an element contained
* by an `ngFor` is rerendered. In this case it is important that changes to
* the `color` attribute do NOT trigger rerenders because doing so will recreate
* and close the colorPicker.
*/
trackByRuns(index: number, data: TableData) {
const dataWithoutColor = {...data};
delete dataWithoutColor.color;
return JSON.stringify(dataWithoutColor);
}
}
17 changes: 17 additions & 0 deletions tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts
Expand Up @@ -355,4 +355,21 @@ describe('runs_data_table', () => {

expect(onRegexFilterChangeSpy).toHaveBeenCalledWith('myRegex');
});

it('trackByRuns serializes data while ignoring color', () => {
const fixture = createComponent({});
const dataTable = fixture.debugElement.query(By.directive(RunsDataTable));
expect(
dataTable.componentInstance.trackByRuns(0, {
id: 'run1',
color: 'orange',
hparam1: 1.234,
})
).toEqual(
JSON.stringify({
id: 'run1',
hparam1: 1.234,
})
);
});
});

0 comments on commit 200ba5e

Please sign in to comment.