Skip to content
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.

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

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

Merged
merged 5 commits into from
Apr 29, 2024

Conversation

hoonji
Copy link
Member

@hoonji hoonji commented Apr 22, 2024

Motivation for features / changes

  • Moving the expand button out of the runs table is required to make it compatible with the upcoming sticky add column b/332788091
  • When the table is both vertically long and horizontally wide, it's very difficult to scroll horizontally: users must scroll all the way to the bottom of the table in order to access the horizontal scroll bar. b/332720882

Technical description of changes

  • Moves the "expand button" from inside the runs data table to the LayoutContainer (in the side bar).
  • Moves the "overflow" css property from the runs table to RunsSelectorComponent (the runs table wrapper)
  • Makes the runs table header sticky. This was already set, but wasn't being applied due to a missing "position: relative" parent.

Screenshots of UI changes (or N/A)

sticky header:
image

expand table button:
image

}

.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

color: inherit;
contain: content;
cursor: pointer;
.full-screen-toggle {
Copy link
Member Author

@hoonji hoonji Apr 22, 2024

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

@@ -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

: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.

@hoonji hoonji requested a review from bmd3k April 22, 2024 07:30
@@ -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

@@ -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

Base automatically changed from memoize_column_fn to master April 24, 2024 11:19
@hoonji hoonji merged commit e9d0009 into master Apr 29, 2024
16 checks passed
@hoonji hoonji deleted the fix_scroll_final branch April 29, 2024 06:25
AnuarTB pushed a commit to AnuarTB/tensorboard that referenced this pull request May 3, 2024
…r scroll (tensorflow#6837)

## Motivation for features / changes
- Moving the expand button out of the runs table is required to make it
compatible with the upcoming sticky add column b/332788091
- When the table is both vertically long and horizontally wide, it's
very difficult to scroll horizontally: users must scroll all the way to
the bottom of the table in order to access the horizontal scroll bar.
b/332720882

## Technical description of changes
- Moves the "expand button" from inside the runs data table to the
LayoutContainer (in the side bar).
- Moves the "overflow" css property from the runs table to
RunsSelectorComponent (the runs table wrapper)
- Makes the runs table header sticky. This was already set, but wasn't
being applied due to a missing "position: relative" parent.

## Screenshots of UI changes (or N/A)
sticky header:

![image](https://github.com/tensorflow/tensorboard/assets/736199/f2569648-6395-4411-9302-f626b6bd5e9f)

expand table button:

![image](https://github.com/tensorflow/tensorboard/assets/736199/35b2e624-f015-41dc-9114-0cab63b70eef)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants