Bug Fix: Start using expect_angular_legacy_material_checkbox #6547
Conversation
This file contains 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
bmd3k
reviewed
Aug 16, 2023
tensorboard/webapp/angular/BUILD
Outdated
| @@ -80,7 +80,7 @@ tf_ts_library( | |||
|
|
|||
| # This is a dummy rule used as a @angular/material/checkbox dependency. | |||
| tf_ts_library( | |||
| name = "expect_angular_material_checkbox", | |||
There was a problem hiding this comment.
Do you still need a "expect_angular_material_checkbox" for the non-legacy usage?
There was a problem hiding this comment.
Yeah, I imagine I do. I was about to run sync to see if there were any such issues that I had missed but you beat me to it.
| @@ -80,7 +80,7 @@ tf_ts_library( | |||
|
|
|||
| # This is a dummy rule used as a @angular/material/checkbox dependency. | |||
| tf_ts_library( | |||
| name = "expect_angular_material_checkbox", | |||
| name = "expect_angular_legacy_material_checkbox", | |||
There was a problem hiding this comment.
Can you followup with a PR that adds "legacy" to the names of all other relevant targets in this file?
There was a problem hiding this comment.
@JamesHollyer and I believe we have found a relatively simple solution to update all the components to the non legacy variants. James is planning on updating all the components, then updating the build rule definitions before syncing them all in at once. I have gone ahead and made the change on my fork but will wait to create a PR rileyajones@0029b45
bmd3k
approved these changes
Aug 16, 2023
rileyajones
added a commit
that referenced
this pull request
Aug 24, 2023
## Motivation for features / changes All the material design components are actually being remapped to the legacy variants internally this is confusing and has already lead to issues like the one being fixed by #6547. Googlers see [cl/558173803](http://cl/558173803) for the build rules being added internally. See [cl/559814988](http://cl/559814988) for confidence this will not break sync
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this pull request
Aug 25, 2023
…low#6547) ## Motivation for features / changes tensorflow#6493 Broke our internal sync Googlers see [cl/557499468](http://cl/557499468) The issue is related to our internal Angular component migration. New components are not allowed to rely on the legacy components. Due to this the new component added in tensorflow#6493 uses the non legacy checkbox. However, our dependency remapping always remaps dependencies on material_checkbox to legacy_material_checkbox. To fix this I am changing the name of the remapping rule in [cl/557515881](http://cl/557515881) and adjusting all the OSS usage EXCEPT for the usage in filter_dialog. [cl/557590954](http://cl/557590954) is also required to map the testing dependency needed to use `MatCheckboxHarness`
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this pull request
Aug 25, 2023
## Motivation for features / changes All the material design components are actually being remapped to the legacy variants internally this is confusing and has already lead to issues like the one being fixed by tensorflow#6547. Googlers see [cl/558173803](http://cl/558173803) for the build rules being added internally. See [cl/559814988](http://cl/559814988) for confidence this will not break sync
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
#6493 Broke our internal sync Googlers see cl/557499468
The issue is related to our internal Angular component migration. New components are not allowed to rely on the legacy components. Due to this the new component added in #6493 uses the non legacy checkbox. However, our dependency remapping always remaps dependencies on material_checkbox to legacy_material_checkbox.
To fix this I am changing the name of the remapping rule in cl/557515881 and adjusting all the OSS usage EXCEPT for the usage in filter_dialog.
cl/557590954 is also required to map the testing dependency needed to use
MatCheckboxHarness