Skip to content

Commit 0246da2

Browse files
authored
Bug Fix: Emit correct timeSelection when prospective fob is clicked (#6054)
* Motivation for features / changes When clicking a prospective fob with a value less than the existing fob both values are set as the maximum. This only happens when linked time is enabled. For Googlers https://screenshot.googleplex.com/XbTQfkKUaPkkRGM * Technical description of changes This seems to have been caused by setting the start step to a value which is greater than a end step. * Screenshots of UI changes Look! It's fixed! ![26033d1e-dfb0-4205-aee3-992ebbdf6b40](https://user-images.githubusercontent.com/78179109/202323362-50efa253-01ca-49d1-b023-90e58a97270d.gif) * Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Navigate to http://localhost:6006?enableLinkedTIme&enableRangeSelection&enableProspectiveFob 3) Enable Linked Time in the settings panel 4) View a scalar card and click someplace on the X Axis to enable step selection 5) Click someplace to the left of the current fob to enable range selection 6) Assert that a new fob appears where you clicked * Alternate designs / implementations considered
1 parent 9741f71 commit 0246da2

File tree

2 files changed

+55
-13
lines changed

2 files changed

+55
-13
lines changed

tensorboard/webapp/widgets/card_fob/card_fob_controller_component.ts

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -382,20 +382,10 @@ export class CardFobControllerComponent {
382382

383383
prospectiveFobClicked(event: Event) {
384384
event.stopPropagation();
385-
if (this.prospectiveStep === null) {
385+
const newTimeSelection = this.getProspectiveTimeSelection();
386+
if (!newTimeSelection) {
386387
return;
387388
}
388-
const newTimeSelection = this.timeSelection
389-
? {
390-
...this.timeSelection,
391-
end: {
392-
step: this.prospectiveStep,
393-
},
394-
}
395-
: {
396-
start: {step: this.prospectiveStep},
397-
end: null,
398-
};
399389

400390
this.onTimeSelectionChanged.emit({
401391
affordance: TimeSelectionAffordance.FOB_ADDED,
@@ -404,6 +394,31 @@ export class CardFobControllerComponent {
404394
this.onPrespectiveStepChanged.emit(null);
405395
}
406396

397+
private getProspectiveTimeSelection() {
398+
if (!this.prospectiveStep) {
399+
return;
400+
}
401+
if (this.timeSelection) {
402+
const startStep = Math.min(
403+
this.timeSelection.start.step,
404+
this.prospectiveStep
405+
);
406+
const endStep = Math.max(
407+
this.timeSelection.start.step,
408+
this.prospectiveStep
409+
);
410+
return {
411+
start: {step: startStep},
412+
end: {step: endStep},
413+
};
414+
}
415+
416+
return {
417+
start: {step: this.prospectiveStep},
418+
end: null,
419+
};
420+
}
421+
407422
/**
408423
* When in range selection(which means we have a start and an end
409424
* fob) clicking "X" to remove a fob will leave the remaining fob in place.

tensorboard/webapp/widgets/card_fob/card_fob_controller_test.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1347,7 +1347,7 @@ describe('card_fob_controller', () => {
13471347
expect(prospectiveFob).toBeTruthy();
13481348
});
13491349

1350-
it('does not render when feature flag is disenabled', () => {
1350+
it('does not render when feature flag is disabled', () => {
13511351
const fixture = createComponent({
13521352
timeSelection: {start: {step: 4}, end: null},
13531353
isProspectiveFobFeatureEnabled: false,
@@ -1407,6 +1407,33 @@ describe('card_fob_controller', () => {
14071407
expect(prospectiveFobTopPosition).toEqual(2);
14081408
});
14091409

1410+
describe('builds timeSelection correctly', () => {
1411+
it('when prospective step is less than timeSelection.start.step', () => {
1412+
const fixture = createComponent({
1413+
timeSelection: {start: {step: 4}, end: null},
1414+
axisDirection: AxisDirection.VERTICAL,
1415+
isProspectiveFobFeatureEnabled: true,
1416+
prospectiveStep: 2,
1417+
});
1418+
fixture.detectChanges();
1419+
1420+
fixture.debugElement
1421+
.query(By.css('.prospective-fob-area'))
1422+
.nativeElement.click();
1423+
expect(onTimeSelectionChanged.calls.allArgs()).toEqual([
1424+
[
1425+
{
1426+
timeSelection: {
1427+
start: {step: 2},
1428+
end: {step: 4},
1429+
},
1430+
affordance: TimeSelectionAffordance.FOB_ADDED,
1431+
},
1432+
],
1433+
]);
1434+
});
1435+
});
1436+
14101437
describe('prospective area', () => {
14111438
it('emits null step on mouseleave', () => {
14121439
const fixture = createComponent({

0 commit comments

Comments
 (0)