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

Makes data table add column sticky #6839

Merged
merged 8 commits into from
Apr 29, 2024
Merged

Makes data table add column sticky #6839

merged 8 commits into from
Apr 29, 2024

Conversation

hoonji
Copy link
Member

@hoonji hoonji commented Apr 22, 2024

Motivation for features / changes

The add column button in the runs table will be hidden when the table is wide, which will often be the case. This makes the "add" feature very difficult to discover.

context: b/332788091

Technical description of changes

  • Moves the add button column outside of and adjacent to the data table (previously it was a column of the table) and makes it sticky
  • Adds additional configuration options to data table to allow runs and scalar tables to have slightly different designs

Screenshots of UI changes (or N/A)

Notice the add column is fixed to the right:
sticky

Scalar table add column is slightly narrower:
image

Alternate designs / implementations considered (or N/A)

  • Tried adding the add columns to the consumer components of the data table (runs data table and scalar card data table) instead. But this led to too much code duplication in the components and templates.
  • Tried making the add column a regular table column, instead of adding it outside in a separate div. But making the column sticky requires styling the table rows, which data table doesn't provide access to (rows are configured solely in the consumer components). Also, the chosen implementation is arguably more consistent with the current design as the add "column" doesn't actually function as a table column at all

@hoonji hoonji requested a review from bmd3k April 22, 2024 12:01
@hoonji
Copy link
Member Author

hoonji commented Apr 22, 2024

This needs testing, but I think screenshot tests will more appropriate here (rather than programmatically testing css structure, which is subject to change). PLMK if the code looks good, and I'll add some 1p tests before submitting this.

Copy link
Contributor

@bmd3k bmd3k left a comment

Choose a reason for hiding this comment

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

I feel a little underqualified to review this PR. I think the change is fine but I recognize that Riley and James spent much more time discussing this button than I did. I defer to them.

Copy link
Contributor

@rileyajones rileyajones left a comment

Choose a reason for hiding this comment

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

I'm totally fine with your approach to making this button sticky and I do like it being sticky.
My biggest feedback is that making the entire column stick exacerbates the white space issue.

One potential solution would be to make just the button sticky and add space at the end of the table so that it can be scrolled out of the table. I'm not sure if that would end up feeling kludgy though so I'll leave it to your judgement.

class="add-button-cell"
*ngIf="selectableColumns && selectableColumns.length"
class="add-button-column"
[class.small-add-button]="addColumnSize == AddColumnSize.SMALL"
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick:

Suggested change
[class.small-add-button]="addColumnSize == AddColumnSize.SMALL"
[class.small-add-button]="addColumnSize === AddColumnSize.SMALL"

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

.add-button {
// Override very high specificity with !important
// Ref: https://github.com/tensorflow/tensorboard/blob/f2e8bd82a533c175f6dc5be3da65344d719ae0f6/tensorboard/webapp/theme/_tb_theme.template.scss#L320
width: 20px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty opposed to using !important as it eventually spirals into an arms race.
If you updated the template to use a css variable, then changed the value of the variable here you wouldn't have to use !important.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh this is great, done!

@hoonji
Copy link
Member Author

hoonji commented Apr 24, 2024

One potential solution would be to make just the button sticky and add space at the end of the table so that it can be scrolled out of the table

Loved this idea at first:

image

But after trying it out, it indeed looks & feels a bit hacky, and it also unfortunately breaks the vertical stickyness (sticky seems to be very brittle to minor changes in general).

Thus, IMO the column with whitespace is the lesser of evils and may be the best we can do atm without dedicated design support.

@hoonji hoonji requested a review from rileyajones April 24, 2024 14:28
Base automatically changed from fix_scroll_final to master April 29, 2024 06:25
@hoonji hoonji merged commit ad1da0c into master Apr 29, 2024
16 checks passed
@hoonji hoonji deleted the add_column_final branch April 29, 2024 09:09
AnuarTB pushed a commit to AnuarTB/tensorboard that referenced this pull request May 3, 2024
## Motivation for features / changes
The add column button in the runs table will be hidden when the table is
wide, which will often be the case. This makes the "add" feature very
difficult to discover.

context: b/332788091

## Technical description of changes
- Moves the add button column outside of and adjacent to the data table
(previously it was a column of the table) and makes it sticky
- Adds additional configuration options to data table to allow runs and
scalar tables to have slightly different designs

## Screenshots of UI changes (or N/A)
Notice the add column is fixed to the right:

![sticky](https://github.com/tensorflow/tensorboard/assets/736199/0f7aca5c-0903-438a-bafd-ddc033e262f7)

Scalar table add column is slightly narrower:

![image](https://github.com/tensorflow/tensorboard/assets/736199/e8c37505-4c3f-41bd-98f3-3b31f99e206e)

## Alternate designs / implementations considered (or N/A)
- Tried adding the add columns to the consumer components of the data
table (runs data table and scalar card data table) instead. But this led
to too much code duplication in the components and templates.
- Tried making the add column a regular table column, instead of adding
it outside in a separate div. But making the column sticky requires
styling the table rows, which data table doesn't provide access to (rows
are configured solely in the consumer components). Also, the chosen
implementation is arguably more consistent with the current design as
the add "column" doesn't actually function as a table column at all
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.

3 participants