Skip to content

[Runs table] Moves expand button to layout container and fixes sidebar scroll #6837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 29, 2024
Merged

[Runs table] Moves expand button to layout container and fixes sidebar scroll #6837

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
627d64d
Memoizes column header functions
hoonji Apr 21, 2024
24d5504
Fixes test
hoonji Apr 21, 2024
98f0260
Merge branch 'master' into memoize_column_fn
hoonji Apr 22, 2024
e11681d
Moves expand button to layout container and fixes sidebar scroll
hoonji Apr 22, 2024
781e980
Remove unused css variable
hoonji Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 57 additions & 15 deletions tensorboard/webapp/core/views/layout_container.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,34 @@ limitations under the License.

.sidebar {
max-width: 80vw;
position: relative;
}

.resizer,
.expand {
.expand-collapsed-sidebar {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to avoid conflict with .full-screen-toggle's .expand

@include tb-theme-foreground-prop(border-color, border);
box-sizing: border-box;
flex: 0 0;
justify-self: stretch;
}

.expand {
.expand-collapsed-sidebar {
width: 20px;
align-items: center;
background: transparent;
border-style: solid;
border-width: 0 1px 0 0;
color: inherit;
contain: content;
cursor: pointer;
display: flex;
justify-self: stretch;
padding: 0;

mat-icon {
transform: rotate(-90deg);
transform-origin: center;
}
}

.resizer {
Expand Down Expand Up @@ -63,20 +79,46 @@ limitations under the License.
}
}

.expand {
align-items: center;
background: transparent;
border-style: solid;
border-width: 0 1px 0 0;
color: inherit;
contain: content;
cursor: pointer;
.full-screen-toggle {
Copy link
Member Author

@hoonji hoonji Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from runs_data_table.css

(with minor edit to fix bug in current style):
image

opacity: 0;
position: absolute;
height: 100%;
// Ensure the button is on the right side then add 2px for the drag target.
left: calc(100% + 2px);
top: 0;
z-index: 1;
display: flex;
justify-self: stretch;
padding: 0;
align-items: center;

mat-icon {
transform: rotate(-90deg);
transform-origin: center;
&:hover {
opacity: 0.8;
}

&.full-screen {
left: unset;
right: 0;
}

.full-screen-btn {
$_arrow_size: 16px;
$_arrow_button_size: calc($_arrow_size + 4px);

background-color: gray;
padding: 0;
min-width: $_arrow_button_size;
width: $_arrow_button_size;

&.expand {
border-radius: 0 $_arrow_button_size $_arrow_button_size 0;
}

&.collapse {
border-radius: $_arrow_button_size 0 0 $_arrow_button_size;
}

.expand-collapse-icon {
font-size: $_arrow_size;
margin-right: 0; // Removes default
}
}
}
28 changes: 26 additions & 2 deletions tensorboard/webapp/core/views/layout_container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {Store} from '@ngrx/store';
import {fromEvent, Observable, Subject} from 'rxjs';
import {combineLatestWith, filter, map, takeUntil} from 'rxjs/operators';
import {MouseEventButtons} from '../../util/dom';
import {sideBarWidthChanged} from '../actions';
import {runsTableFullScreenToggled, sideBarWidthChanged} from '../actions';
import {State} from '../state';
import {
getRunsTableFullScreen,
Expand All @@ -34,7 +34,7 @@ import {
template: `
<button
*ngIf="(width$ | async) === 0"
class="expand"
class="expand-collapsed-sidebar"
(click)="expandSidebar()"
>
<mat-icon svgIcon="expand_more_24px"></mat-icon>
Expand All @@ -47,6 +47,26 @@ import {
[style.maxWidth.%]="(runsTableFullScreen$ | async) ? 100 : ''"
>
<ng-content select="[sidebar]"></ng-content>
<div
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied from runs_data_table.ng.html

class="full-screen-toggle"
[ngClass]="{'full-screen': (runsTableFullScreen$ | async)}"
>
<button
mat-button
class="full-screen-btn"
[ngClass]="(runsTableFullScreen$ | async) ? 'collapse' : 'expand'"
(click)="toggleFullScreen()"
>
<mat-icon
class="expand-collapse-icon"
[svgIcon]="
(runsTableFullScreen$ | async)
? 'arrow_back_24px'
: 'arrow_forward_24px'
"
></mat-icon>
</button>
</div>
</nav>
<div
*ngIf="(width$ | async) > 0"
Expand Down Expand Up @@ -125,4 +145,8 @@ export class LayoutContainer implements OnDestroy {
})
);
}

toggleFullScreen() {
this.store.dispatch(runsTableFullScreenToggled());
}
}
2 changes: 1 addition & 1 deletion tensorboard/webapp/core/views/layout_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ describe('layout test', () => {
let dispatchedActions: Action[] = [];

const byCss = {
EXPANDER: By.css('.expand'),
EXPANDER: By.css('.expand-collapsed-sidebar'),
RESIZER: By.css('.resizer'),
SIDEBAR_CONTAINER: By.css('nav'),
LAYOUT: By.directive(LayoutContainer),
Expand Down
1 change: 1 addition & 0 deletions tensorboard/webapp/metrics/views/card_renderer/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ tf_ng_module(
":utils",
"//tensorboard/webapp/metrics:types",
"//tensorboard/webapp/runs:types",
"//tensorboard/webapp/util:memoize",
"//tensorboard/webapp/widgets/card_fob:types",
"//tensorboard/webapp/widgets/data_table",
"//tensorboard/webapp/widgets/data_table:types",
Expand Down
4 changes: 2 additions & 2 deletions tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
(loadAllColumns)="loadAllColumns.emit()"
>
<ng-container header>
<ng-container *ngFor="let header of getHeaders()">
<ng-container *ngFor="let header of extendHeaders(columnHeaders)">
<tb-data-table-header-cell
*ngIf="header.enabled && (header.type !== ColumnHeaderType.SMOOTHED || smoothingEnabled)"
[header]="header"
Expand All @@ -42,7 +42,7 @@
<ng-container content>
<ng-container *ngFor="let dataRow of getTimeSelectionTableData()">
<tb-data-table-content-row>
<ng-container *ngFor="let header of getHeaders()">
<ng-container *ngFor="let header of extendHeaders(columnHeaders)">
<tb-data-table-content-cell
*ngIf="header.enabled && (header.type !== ColumnHeaderType.SMOOTHED || smoothingEnabled)"
[header]="header"
Expand Down
27 changes: 18 additions & 9 deletions tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
AddColumnEvent,
} from '../../../widgets/data_table/types';
import {isDatumVisible} from './utils';
import {memoize} from '../../../util/memoize';

@Component({
selector: 'scalar-card-data-table',
Expand Down Expand Up @@ -74,15 +75,23 @@ export class ScalarCardDataTable {

ColumnHeaderType = ColumnHeaderType;

getHeaders(): ColumnHeader[] {
return [
{
name: 'color',
displayName: '',
type: ColumnHeaderType.COLOR,
enabled: true,
},
].concat(this.columnHeaders);
// Columns must be memoized to stop needless re-rendering of the content and headers in these
// columns. This has been known to cause problems with the controls in these columns,
// specifically the add button.
extendHeaders = memoize(this.internalExtendHeaders);

private internalExtendHeaders(headers: ColumnHeader[]) {
return ([] as Array<ColumnHeader>).concat(
[
{
name: 'color',
displayName: '',
type: ColumnHeaderType.COLOR,
enabled: true,
},
],
headers
);
}

getMinPointInRange(
Expand Down
7 changes: 7 additions & 0 deletions tensorboard/webapp/runs/views/runs_selector/runs_selector_component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ import {RunsTableColumn} from '../runs_table/types';
`,
styles: [
`
:host {
display: block;
height: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor preference: If we leave height unset then for fewer runs the horizontal scrollbar will be closer to the runs instead at the bottom of the page.

Up to you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this out. Unfortunately this also prevents vertical scroll entirely on longer tables - the height 100% is required to constrain the long table height to enable scrolling

width: 100%;
overflow: auto;
}

runs-table {
height: 100%;
}
Expand Down
1 change: 1 addition & 0 deletions tensorboard/webapp/runs/views/runs_table/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ tf_ng_module(
"//tensorboard/webapp/types:ui",
"//tensorboard/webapp/util:colors",
"//tensorboard/webapp/util:matcher",
"//tensorboard/webapp/util:memoize",
"//tensorboard/webapp/widgets/custom_modal",
"//tensorboard/webapp/widgets/data_table",
"//tensorboard/webapp/widgets/data_table:filter_dialog",
Expand Down
17 changes: 2 additions & 15 deletions tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
(loadAllColumns)="loadAllColumns.emit()"
>
<ng-container header>
<ng-container *ngFor="let header of getHeaders()">
<ng-container *ngFor="let header of extendHeaders(headers)">
<tb-data-table-header-cell
[header]="header"
[sortingInfo]="sortingInfo"
Expand All @@ -67,7 +67,7 @@
<ng-container content>
<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()">
<ng-container *ngFor="let header of extendHeaders(headers)">
<tb-data-table-content-cell
[header]="header"
[datum]="dataRow[header.name]"
Expand Down Expand Up @@ -106,16 +106,3 @@
</ng-container>
</tb-data-table>
</div>
<div class="full-screen-toggle" [ngClass]="{'full-screen': isFullScreen}">
<button
mat-button
class="full-screen-btn"
[ngClass]="isFullScreen ? 'collapse' : 'expand'"
(click)="toggleFullScreen.emit()"
>
<mat-icon
class="expand-collapse-icon"
[svgIcon]="isFullScreen ? 'arrow_back_24px' : 'arrow_forward_24px'"
></mat-icon>
</button>
</div>
54 changes: 4 additions & 50 deletions tensorboard/webapp/runs/views/runs_table/runs_data_table.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,11 @@ $_circle-size: 20px;
$_arrow_size: 16px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can delete $_arrow_size definition here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


:host {
width: 100%;
}
min-width: 100%;

:host {
overflow-y: auto;
width: 100%;
& ::ng-deep tb-data-table .data-table {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces the previous border-bottom of the .filter-row. It ensures that the entire table has a border-top, instead of just the part touching the filter row.

@include tb-theme-foreground-prop(border-top, border, 1px solid);
}
}

.color-container {
Expand Down Expand Up @@ -72,49 +71,7 @@ tb-data-table-header-cell:last-of-type {
width: 40px;
}

.full-screen-toggle {
opacity: 0;
position: absolute;
height: 100%;
// Ensure the button is on the right side then add 2px for the drag target.
left: calc(100% + 2px);
top: 0;
z-index: 1;
display: flex;
align-items: center;

&:hover {
opacity: 0.8;
}

&.full-screen {
left: unset;
right: 0;
}

.full-screen-btn {
background-color: gray;
padding: 0;
min-width: $_arrow_size;
width: $_arrow_size;

&.expand {
border-radius: 0 $_arrow_size $_arrow_size 0;
}

&.collapse {
border-radius: $_arrow_size 0 0 $_arrow_size;
}

.expand-collapse-icon {
font-size: $_arrow_size;
width: $_arrow_size;
}
}
}

.filter-row {
@include tb-theme-foreground-prop(border-bottom, border, 1px solid);
display: flex;
align-items: center;
height: 48px;
Expand All @@ -125,6 +82,3 @@ tb-data-table-header-cell:last-of-type {
flex-grow: 1;
}
}
.table-container {
overflow-x: auto;
}
Loading
Loading