Add directives to modify attributes and include feature flags where needed #6341
Conversation
groszewn
force-pushed
the
client_feature_flags
branch
from
6a98fb0
to
e8f8c5e
Compare
tensorboard/webapp/metrics/views/card_renderer/image_card_directive.ts
Outdated
Show resolved
Hide resolved
tensorboard/webapp/metrics/views/card_renderer/image_card_directive.ts
Outdated
Show resolved
Hide resolved
tensorboard/webapp/metrics/views/card_renderer/image_card_directive.ts
Outdated
Show resolved
Hide resolved
tensorboard/webapp/metrics/views/card_renderer/image_card_directive.ts
Outdated
Show resolved
Hide resolved
groszewn
force-pushed
the
client_feature_flags
branch
6 times, most recently
from
206e409
to
78afa74
Compare
|
@bmd3k thanks for the offline help, tests should be passing now with the singular directive. |
| declarations: [FeatureFlagDirective], | ||
| exports: [FeatureFlagDirective], | ||
| }) | ||
| export class FeatureFlagDirectiveModule {} |
There was a problem hiding this comment.
Usually we keep the NgModule in a seperate file.
| export class FeatureFlagDirective { | ||
| @HostBinding('attr.href') hrefAttr: string | undefined = undefined; | ||
| @HostBinding('attr.src') srcAttr: string | undefined = undefined; | ||
| @Input() includeFeatureFlags: boolean = true; |
There was a problem hiding this comment.
This is a little confusing.
[includeFeatureFlags]="false", for example, won't actually have any impact on the logic. It will behave exactly the same as though [includeFeatureFlags] is unset or [includeFeatureFlags]="true".
I think that's fine for now (and maybe forever) but it would be good to acknowledge that with a comment.
| @@ -0,0 +1,136 @@ | |||
| /* Copyright 2022 The TensorFlow Authors. All Rights Reserved. | |||
| class TestableHrefComponent { | ||
| @ViewChild( | ||
| TestComponent | ||
| ) /* using viewChild we get access to the TestComponent which is a child of TestHostComponent */ |
There was a problem hiding this comment.
IMO these comments are oddly located and should appear before the entire statement that defines and annotates the variable. (so this would move to L47, for example, and the ViewChild annotation would move to L48.)
There was a problem hiding this comment.
No longer relevant/necessary, simplified the test setup.
| class TestableHrefComponent { | ||
| @ViewChild( | ||
| TestComponent | ||
| ) /* using viewChild we get access to the TestComponent which is a child of TestHostComponent */ |
There was a problem hiding this comment.
Nit: Comments are statements or sentences to the best of our ability. Specifically this means you Capitalize the first letter in the comment and you put some punctuation at the end of it.
There was a problem hiding this comment.
Ack, this setup has been simplified so we no longer have this.
|
|
||
| it('injects feature flags in <img> tags if any are set', fakeAsync(() => { | ||
| store.overrideSelector(getFeatureFlagsToSendToServer, {inColab: true}); | ||
| const anchorStr = createImgComponent('https://abc.def'); |
There was a problem hiding this comment.
You should also test the case where there are already query parameters since that is a case handled in your class.
| selector: 'test', | ||
| template: `<p>{{ value }}</p>`, | ||
| }) | ||
| export class TestComponent { |
There was a problem hiding this comment.
I don't understand why TestComponent exists. Can it be removed from the tests?
L94 has:
const component = hostComponent.testComponent;
But nothing is done with component.
There was a problem hiding this comment.
This setup was necessary with the initial implementation (since ngOnChanges was used, but detectChanges wasn't invoking the lifecycle hook). I've simplified the setup to remove this.
| class TestableHrefComponent { | ||
| @ViewChild( | ||
| TestComponent | ||
| ) /* using viewChild we get access to the TestComponent which is a child of TestHostComponent */ |
There was a problem hiding this comment.
No longer needed, but that was a copy/paste error.
|
|
||
| it('leaves <a> tags unmodified if no feature flags are set', fakeAsync(() => { | ||
| const anchorStr = createHrefComponent('https://abc.def'); | ||
| expect(anchorStr.attributes['href']).toBe('https://abc.def'); |
There was a problem hiding this comment.
For what it's worth, merging the <a> and <img> versions of this directive into one was meant to limit some of this duplication in tests. Assuming you thoroughly test <img> tags then I think you could get away with just a single test for <a> tags - mostly like the one currently on line 124.
| it('leaves <a> tags unmodified if no feature flags are set', fakeAsync(() => { | ||
| const anchorStr = createHrefComponent('https://abc.def'); | ||
| expect(anchorStr.attributes['href']).toBe('https://abc.def'); | ||
| })); |
There was a problem hiding this comment.
Do you mind adding tests where the a or img tag does not contain [includeFeatureFlags] attribute?
…ecessary Updates individual HTML elements to include client feature flags as query parameters on their src attributes through the use of a directive. Validated via a local tensorboard that the attributes generated from these directives include the client feature flags as query parameters.
groszewn
force-pushed
the
client_feature_flags
branch
from
78afa74
to
dce55df
Compare
groszewn
force-pushed
the
client_feature_flags
branch
from
dce55df
to
2e61f09
Compare
Updates individual HTML elements to include client feature flags as query parameters on their src attributes through the use of a directive.
Validated via a local tensorboard that the attributes generated from these directives include the client feature flags as query parameters.