Skip to content

Commit 068843a

Browse files
authored
Linked Time: Update prospective fob styling (#6055)
## Motivation for features / changes 1) The text in the prospective fob was off center 2) The prospective fob "did not feel clickable" 3) Clicking near the prospective fob would enable it which lead to some weird bugs ## Technical description of changes I changed the padding on the fob when the `allowRemoval` flag is set to fix the text centering issuel The prospective fob is shown based on the value of the `prospectiveStep` variable which is set when hovering the `prospective-fob-area` and set to `null` when the cursor leaves that area. I moved the prospectiveFob inside the `prospective-fob-area` so the cursor would not count as leaving it when the fob appears and takes focus. I also moved the click handler to the fob object (rather than the `prospective-fob-area`) to ensure click events are more accurate. The changes to the HTML nesting made css changes much easier allowing the fob change opacity when hovered to draw additional attention to it and the cursor should not properly change. ## Screenshots of UI changes Before: ![image](https://user-images.githubusercontent.com/78179109/202324386-5786a0df-67f2-40a3-8bbf-2e9f77c48cd2.png) After: Hovering Area (Not fob) ![image](https://user-images.githubusercontent.com/78179109/202561500-eaa7003b-5c9c-4d24-bdf4-33c41224165e.png) Hovering Fob ![image](https://user-images.githubusercontent.com/78179109/202561589-f2b70428-2800-4eb4-917f-93432f87c156.png) ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Go to http://localhost:6006?enableDataTable=true&allowRangeSelection=true&enableProspectiveFob=true#timeseries 3) Hover the X Axis of the scalar card and assert a prospective fob appears 4) Hover the prospective fob and assert the opacity and cursor change 5) Click the prospective fob and assert a start fob is added 6) Repeat steps 3-5 and assert an end fob is added 7) Hover the XAxis and assert no prospective fob is shown ## Alternate designs / implementations considered I considered increasing the size of the `prospective-fob-area` and I also considered making the prospective fob larger when hovered.
1 parent 471f1f2 commit 068843a

File tree

8 files changed

+75
-32
lines changed

8 files changed

+75
-32
lines changed

tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3455,20 +3455,23 @@ describe('scalar card', () => {
34553455
expect(fobs.length).toEqual(1);
34563456

34573457
// Click the prospective fob to set the start time
3458-
fixture.debugElement
3459-
.query(By.css('.prospective-fob-area'))
3460-
.nativeElement.click();
3458+
testController.prospectiveFobClicked(new MouseEvent('mouseclick'));
3459+
fixture.detectChanges();
3460+
3461+
// One start fob
3462+
fobs = fixture.debugElement.queryAll(By.directive(CardFobComponent));
3463+
expect(fobs.length).toEqual(2);
34613464
fixture.detectChanges();
34623465

34633466
// One start fob + 1 prospective fob
3467+
testController.prospectiveStep = 25;
3468+
fixture.detectChanges();
3469+
34643470
fobs = fixture.debugElement.queryAll(By.directive(CardFobComponent));
34653471
expect(fobs.length).toEqual(2);
34663472

34673473
// Click the prospective fob to set the end time
3468-
testController.prospectiveStep = 25;
3469-
fixture.debugElement
3470-
.query(By.css('.prospective-fob-area'))
3471-
.nativeElement.click();
3474+
testController.prospectiveFobClicked(new MouseEvent('mouseclick'));
34723475
fixture.detectChanges();
34733476

34743477
// One start fob, one end fob

tensorboard/webapp/widgets/card_fob/card_fob_component.ng.html

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@
1515
limitations under the License.
1616
-->
1717

18-
<div class="fob">
18+
<div
19+
[ngClass]="{'fob': true, 'unremovable': !allowRemoval, 'prospective': isProspective}"
20+
>
1921
<span
2022
contenteditable
2123
#stepSpan
@@ -34,4 +36,11 @@
3436
>
3537
<mat-icon svgIcon="close_24px"></mat-icon>
3638
</button>
39+
<button
40+
aria-label="Deselect fob"
41+
(click)="fobRemoved.emit()"
42+
*ngIf="isProspective"
43+
>
44+
<mat-icon svgIcon="keep_24px"></mat-icon>
45+
</button>
3746
</div>

tensorboard/webapp/widgets/card_fob/card_fob_component.scss

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,35 @@ limitations under the License.
2525
border-radius: 25px;
2626
padding: 2px 2px 2px 4px;
2727
font-size: 11px;
28+
text-align: center;
2829
width: min-content;
2930

31+
> .prospective {
32+
padding-top: 1px;
33+
}
34+
3035
&:hover {
3136
cursor: grab;
37+
38+
&.prospective {
39+
cursor: pointer;
40+
}
3241
}
3342
&:active {
3443
cursor: grabbing;
3544
}
45+
46+
&.unremovable {
47+
padding: 2px 4px;
48+
}
49+
50+
&.prospective {
51+
align-items: center;
52+
box-sizing: border-box;
53+
border: 1px dashed mat.get-color-from-palette(mat.$gray-palette, 500);
54+
font-weight: bold;
55+
height: 17px;
56+
}
3657
}
3758

3859
span {

tensorboard/webapp/widgets/card_fob/card_fob_component.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ export class CardFobComponent {
3838

3939
@Input() allowRemoval?: boolean = true;
4040

41+
@Input() isProspective?: boolean = false;
42+
4143
@Output() stepChanged = new EventEmitter<number | null>();
4244
@Output() fobRemoved = new EventEmitter();
4345

tensorboard/webapp/widgets/card_fob/card_fob_controller_component.ng.html

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,30 +15,36 @@
1515
limitations under the License.
1616
-->
1717

18-
<div>
18+
<div class="card-fob-controller">
1919
<ng-container
2020
*ngIf="isProspectiveFobFeatureEnabled && !rangeSelectionEnabled"
2121
>
2222
<div
23-
#prospectiveFobWrapper
24-
*ngIf="prospectiveStep !== null"
25-
class="time-fob-wrapper prospectiveFob"
23+
*ngIf="showExtendedLine && prospectiveStep !== null"
24+
class="extended-line"
2625
[style.transform]="getCssTranslatePxForProspectiveFob()"
27-
>
28-
<div *ngIf="showExtendedLine" class="extended-line"></div>
29-
<card-fob
30-
[ngClass]="isVertical() ? 'vertical-fob' : 'horizontal-fob'"
31-
[allowRemoval]="false"
32-
[step]="prospectiveStep"
33-
></card-fob>
34-
</div>
26+
></div>
3527
<div
3628
class="prospective-fob-area"
3729
[ngClass]="isVertical() ? 'vertical-prospective-area' : 'horizontal-prospective-area'"
38-
(mousemove)="mouseOver($event)"
30+
(mousemove)="mouseOverProspectiveFobArea($event)"
3931
(mouseleave)="onProspectiveAreaMouseLeave()"
40-
(click)="prospectiveFobClicked($event)"
41-
></div>
32+
>
33+
<div
34+
#prospectiveFobWrapper
35+
*ngIf="prospectiveStep !== null"
36+
(click)="prospectiveFobClicked($event)"
37+
class="time-fob-wrapper"
38+
[style.transform]="getCssTranslatePxForProspectiveFob()"
39+
>
40+
<card-fob
41+
[ngClass]="isVertical() ? 'vertical-fob' : 'horizontal-fob'"
42+
[allowRemoval]="false"
43+
[isProspective]="true"
44+
[step]="prospectiveStep"
45+
></card-fob>
46+
</div>
47+
</div>
4248
</ng-container>
4349
<div
4450
#startFobWrapper

tensorboard/webapp/widgets/card_fob/card_fob_controller_component.scss

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@ limitations under the License.
1616
pointer-events: all;
1717
}
1818

19+
.card-fob-controller {
20+
height: calc(100% + 30px);
21+
position: relative;
22+
}
23+
1924
.time-fob-wrapper {
2025
display: inline-block;
2126
position: absolute;
@@ -51,11 +56,6 @@ limitations under the License.
5156
}
5257
}
5358

54-
.prospectiveFob {
55-
opacity: 0.8;
56-
cursor: pointer;
57-
}
58-
5959
.horizontal-prospective-area {
6060
bottom: 0;
6161
position: absolute;

tensorboard/webapp/widgets/card_fob/card_fob_controller_component.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ export class CardFobControllerComponent {
240240
this.hasFobMoved = true;
241241
}
242242

243-
mouseOver(event: MouseEvent) {
243+
mouseOverProspectiveFobArea(event: MouseEvent) {
244244
if (
245245
this.timeSelection?.end !== null &&
246246
this.timeSelection?.end !== undefined

tensorboard/webapp/widgets/card_fob/card_fob_controller_test.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1417,9 +1417,11 @@ describe('card_fob_controller', () => {
14171417
});
14181418
fixture.detectChanges();
14191419

1420-
fixture.debugElement
1421-
.query(By.css('.prospective-fob-area'))
1422-
.nativeElement.click();
1420+
const testController = fixture.debugElement.query(
1421+
By.directive(CardFobControllerComponent)
1422+
).componentInstance;
1423+
testController.prospectiveFobClicked(new MouseEvent('mouseclick'));
1424+
14231425
expect(onTimeSelectionChanged.calls.allArgs()).toEqual([
14241426
[
14251427
{

0 commit comments

Comments
 (0)