Skip to content

Conversation

@JamesHollyer
Copy link
Contributor

@JamesHollyer JamesHollyer commented May 4, 2023

Motivation for features / changes

These new properties break internal sync as the components which use ColumnHeader do not have these properties.
I will make them required again once internal uses are changed.

Googlers see cl/508442178 for sync test.

@JamesHollyer JamesHollyer requested a review from japie1235813 May 4, 2023 21:11
Copy link
Contributor

@japie1235813 japie1235813 left a comment

Choose a reason for hiding this comment

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

Generally lgtm but could you make a sync test to see that it passes? thanks

name: string;
displayName: string;
name?: string; // TODO: make required after internal changes are make
displayName?: string; // TODO: make required after internal changes are make
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can you add names after TODO?
TODO(jameshollyer): Makes required... made

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@JamesHollyer JamesHollyer merged commit f1fd2a8 into tensorflow:master May 4, 2023
rileyajones pushed a commit to rileyajones/tensorboard that referenced this pull request May 5, 2023
…rflow#6369)

## Motivation for features / changes
These new properties break internal sync as the components which use
ColumnHeader do not have these properties.
I will make them required again once internal uses are changed.

Googlers see cl/508442178 for sync test.
JamesHollyer added a commit that referenced this pull request May 9, 2023
## Motivation for features / changes
These attributes were added in #6346. There were then made optional in
#6369 to fix sync issues. The internal issues have now been resolved and
this changes them back to required fields.
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