HParams: project header cell components #6422
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JamesHollyer
force-pushed
the
ProjectTableHeaders
branch
from
71d4273 to
3bb5c05
Compare
Contributor
|
This is going to require a lot of test updates but I am happy with the approach you have taken. |
rileyajones
reviewed
Jun 8, 2023
| @@ -0,0 +1,44 @@ | |||
| /* Copyright 2022 The TensorFlow Authors. All Rights Reserved. | |||
Contributor
There was a problem hiding this comment.
Suggested change
| /* Copyright 2022 The TensorFlow Authors. All Rights Reserved. | |
| /* Copyright 2023 The TensorFlow Authors. All Rights Reserved. |
| @@ -0,0 +1,54 @@ | |||
| /* Copyright 2022 The TensorFlow Authors. All Rights Reserved. | |||
Contributor
There was a problem hiding this comment.
Is this file getting used?
Contributor
Author
There was a problem hiding this comment.
That was not supposed to be added. I decided to do this in a different PR. Removed.
|
|
||
| syncHeaders() { | ||
| this.headerCells.forEach((headerCell) => { | ||
| headerCell.dragStart.subscribe(this.dragStart.bind(this)); |
Contributor
There was a problem hiding this comment.
Can you ensure thee all get unsubscribed from when the component unmounts
Contributor
Author
There was a problem hiding this comment.
Turns out they were not. I added a structure to track them and ensure there are no leaks.
JamesHollyer
force-pushed
the
ProjectTableHeaders
branch
from
3bb5c05 to
64d2c80
Compare
rileyajones
approved these changes
Jun 9, 2023
JamesHollyer
force-pushed
the
ProjectTableHeaders
branch
from
78c2e1b to
f4f9c34
Compare
JamesHollyer
added a commit
that referenced
this pull request
Jun 15, 2023
## Motivation for features / changes In #6422 we started projecting the header components into the data table widget. However, the content was still not projected and the color column was hard coded in. Therefore we left a hard coded color header. The content started being projected in #6427. Therefore we can stop hardcoding this color column. This PR removes the hardcoded header column and starts adding it from the ScalarCardDataTable ## Technical description of changes The color header cannot be sorted, dismissed, or dragged. There was some logic in place for this. However, it was incomplete. This completes that logic and uses it for the color header. We also add tests for that logic which were not in place before. Note: I did some test clean up in the HeaderCell ## Screenshot of changes It still works. <img width="148" alt="Screenshot 2023-06-15 at 5 41 33 AM" src="https://github.com/tensorflow/tensorboard/assets/8672809/0fe6e62e-f79a-46a8-bf60-cad3d87a28d4"> ComponentTests for consistency. --------- Co-authored-by: Riley Jones <78179109+rileyajones@users.noreply.github.com>
JamesHollyer
added a commit
that referenced
this pull request
Jun 16, 2023
#6438) ## Motivation for features / changes We preparation for adding HParam functionality to both the Scalar Table and the Runs Table we have refactored the Data Table to be more flexible. This refactoring was done in #6422 and #6427 and was only implemented in the Scalar Table because the Runs Table only uses the Data Table behind a flag. This PR implements the new Data Table Structure in the Runs Table. ## Technical description of changes Basically just followed the same structure as the Scalar Table. ## Screenshots of UI changes (or N/A) Runs Table specific styling still needs to be done. It currently looks like this. <img width="392" alt="Screenshot 2023-06-15 at 7 21 46 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/2dd13003-ad93-4b66-89eb-579a497d6708"> We still need to add a column for check boxes and add styling.
JamesHollyer
added a commit
that referenced
this pull request
Jun 22, 2023
## Motivation for features / changes In #6438 we rewrote the Runs Data Table to use the new projection Data Table structure(Created in #6427 and #6422). This allows us to project custom controls such as the run selection check boxes. This PR implements those checkboxes. ## Technical description of changes Pretty straight forward use of projection. The weird part was that the projected checkbox in the header did not fire the "change" event while the header itself had a subscription to a "click" event for sorting. The solution was to subscribe to the "click" event and that seems to work just fine. ## Screenshots of UI changes (or N/A) All selected: <img width="230" alt="Screenshot 2023-06-20 at 2 40 19 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/164ab4c2-b2bd-47ad-b46d-9dbcf78c976a"> Some selected: <img width="228" alt="Screenshot 2023-06-20 at 2 40 27 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/0822e363-ac08-45a3-9102-4c1959192c9f"> None selected: <img width="211" alt="Screenshot 2023-06-20 at 2 40 35 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/9ce0b8aa-182d-4796-a59b-5e05fc655f2e">
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for features / changes
This refactoring allows more customization of the data table which will be used by both the runs table and scalar card. This allows the parents(like the runs table) to add functionality to the header component such as the color pallet and select all checkbox.
Technical description of changes
This change pulls out the header cells from the DataTable and allows the parent component to project them back down. We add a HeaderCellComponent which is used keep consistency and allow the DataTable to still keep the logic which should be on all tables such as dragging and sorting. This is done by using event emitters in the HeaderCellComponent which the DataTable subscribes to.
Screenshots of UI changes (or N/A)
The Scalar table has no changes.
This does break the runs data table which is still behind a flag. This will be fixed in a follow up.
Detailed steps to verify changes work correctly (as executed by you)
Ran it and clicked a round a lot to try and break it.
Alternate designs / implementations considered (or N/A)
We considered a more config based solution where we add lots of options for functionality in the header objects being passed down to the table.