Skip to content

HParams: Make icons buttons in data table header #6594

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

Merged

Conversation

JamesHollyer
Copy link
Contributor

Motivation for features / changes

To allow for tabbing to buttons and for general best practice clickable elements should be buttons. So the 3 dot menu button was changed into a button instead of a div. For sorting the entire header is clickable. We did not want to make the entire header a button so I turned the sorting icon into a button.

I also made some styling changes to spread out the icons and make them easier to click.

Technical description of changes

When I made the sorting icon a button I had to add the click event to make it sort. I also had to stop propagation so the header's click event was not fired.

Screenshots of UI changes (or N/A)

2023-09-22_11-05-35 (1)

@JamesHollyer JamesHollyer force-pushed the RunsHeaderTargetCleanup branch from 99ac5a4 to d9e0023 Compare September 22, 2023 22:31
@JamesHollyer JamesHollyer force-pushed the RunsHeaderTargetCleanup branch from d9e0023 to 7046db6 Compare September 22, 2023 22:32
@JamesHollyer JamesHollyer requested a review from bmd3k September 23, 2023 00:08
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.

Looks and works better. Thanks!

*ngIf="(header.removable || header.sortable) && !disableContextMenu"
class="context-menu-container show-on-hover"
(click)="onContextMenuOpened($event)"
Copy link
Contributor

Choose a reason for hiding this comment

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

When I press "space" on this context menu, the context menu is actually opened in the top-left corner of the runs table rather than next to the location of the button.

image

Is there any way to fix this? Maybe take into account the location of the button when opening the context menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I noticed this too I added a task to fix it in our task list.

@JamesHollyer JamesHollyer merged commit 3584a84 into tensorflow:master Sep 25, 2023
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.

2 participants