HParams: Header Button Styling and Context Menu updates #6471
Conversation
| *ngIf=" | ||
| sortingInfo.order === SortingOrder.ASCENDING && | ||
| sortingInfo.name === contextMenuHeader?.name; | ||
| else descending" |
There was a problem hiding this comment.
If it is currently ascending shouldn't we show the down arrow as clicking this button will make it descending?
There was a problem hiding this comment.
I originally had it that way but I found it confusing to have the arrow in the context menu conflict with the arrow in the header
| <button | ||
| mat-icon-button | ||
| class="context-menu-trigger" | ||
| <mat-icon |
There was a problem hiding this comment.
I think this is supposed to be a button for accessibility. Or you have to add tab indexes and aria labels and all that.
There was a problem hiding this comment.
+1 for accessibility, that is why it was a button originally.
I made it just an icon so that it would be consistent with the sort icon, should I also make the sort icon a button too? Also I thought that people didn't like the ripple which comes with mat-button
tensorboard/webapp/widgets/data_table/header_cell_component_test.ts
Outdated
Show resolved
Hide resolved
| svgIcon="arrow_upward_24px" | ||
| ></mat-icon> | ||
| <ng-template #descending> | ||
| <mat-icon svgIcon="arrow_downward_24px"></mat-icon> |
There was a problem hiding this comment.
This logic around which icon renders should be tested.
| @@ -15,19 +15,37 @@ | |||
| <custom-modal #contextMenu> | |||
| <div class="context-menu"> | |||
| <div | |||
| *ngIf="!canContextMenuRemoveColumn() && !(selectableColumns && selectableColumns.length)" | |||
| *ngIf="!contextMenuHeader?.removable && !contextMenuHeader?.sortable && !canContextMenuInsert()" | |||
There was a problem hiding this comment.
Just realized there is no test for this logic.
Motivation for features / changes
While testing the new hparams in time series feature we got a lot of feedback about the styling and placement of the header buttons.
The main takeaways are:
We also noticed that the sort button was missing from the context menu and discovered that right clicking a header cell logged an error to the console
Screenshots of UI changes (or N/A)
Header Buttons (Dark Mode)
Header Buttons (Light Mode)
Context Menu (Dark Mode)
Context Menu (Light Mode)
No context menu in scalar card
