feat: conditional container widget #348

jake-bassett · 2020-11-11T18:32:40Z

Description

Container widget that allows a datasource to provide one of two different children[] based on a conditional query or check.

Testing

This is not the entire implementation. Just base code to enable an extended data source.

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

codecov · 2020-11-11T18:37:27Z

Codecov Report

Merging #348 (de81fd6) into main (46b4982) will decrease coverage by 0.03%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
- Coverage   86.28%   86.24%   -0.04%     
==========================================
  Files         729      733       +4     
  Lines       14844    14875      +31     
  Branches     1896     1848      -48     
==========================================
+ Hits        12808    12829      +21     
- Misses       2005     2013       +8     
- Partials       31       33       +2

Impacted Files	Coverage Δ
...c/properties/property-types/model-template-type.ts	`83.33% <ø> (ø)`
...aphql/conditional/conditional-data-source.model.ts	`0.00% <0.00%> (ø)`
...oard/data/graphql/table/table-data-source.model.ts	`87.50% <ø> (ø)`
...nditional/conditional-widget-renderer.component.ts	`60.00% <60.00%> (ø)`
...oards/src/widgets/conditional/conditional.model.ts	`66.66% <66.66%> (ø)`
projects/dashboards/src/public-api.ts	`100.00% <100.00%> (ø)`
...c/widgets/conditional/conditional-widget.module.ts	`100.00% <100.00%> (ø)`
...dashboards/src/widgets/dashboard-widgets.module.ts	`100.00% <100.00%> (ø)`
...ects/dashboards/src/widgets/repeat/repeat.model.ts	`38.70% <100.00%> (ø)`
...cts/dashboards/src/widgets/repeat/repeat.module.ts	`100.00% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46b4982...de81fd6. Read the comment docs.

anandtiwary · 2020-11-11T19:16:48Z

projects/dashboards/src/widgets/container/conditional-container-widget.model.ts

+    type: ARRAY_PROPERTY.type,
+    required: true
+  })
+  public trueChildren: object[] = [];


anandtiwary · 2020-11-11T19:17:03Z

projects/dashboards/src/widgets/container/conditional-container-widget.model.ts

+    type: ARRAY_PROPERTY.type,
+    required: true
+  })
+  public falseChildren: object[] = [];


Shhhhh. Don't tell them! 😂

anandtiwary · 2020-11-11T19:18:38Z

projects/dashboards/src/widgets/container/conditional-container-widget.model.ts

+  private readonly api!: ModelApi;
+
+  public getData(): Observable<boolean> {
+    return this.api.getData<boolean>();


Container may not have any data source. So why would this ever emit?

A conditional container widget will require a datasource to determine which children to return.

aaron-steinfeld · 2020-11-11T19:49:43Z

projects/dashboards/src/widgets/container/container-widget-renderer.component.ts

@@ -18,15 +20,17 @@ import { ContainerWidgetModel } from './container-widget.model';
    </div>
  `
 })
-export class ContainerWidgetRendererComponent extends WidgetRenderer<ContainerWidgetModel> implements AfterViewInit {
+export class ContainerWidgetRendererComponent
+  extends WidgetRenderer<ContainerWidgetModel | ConditionalContainerWidgetModel>


So two issues with this approach that I see -

The renderer now has to know about different types of models (not a huge deal)

The conditional works at a container level - this seems more problematic. If I want to condition one widget (which is more likely the case), does that mean it gets padded? What if the condition should change the layout? In my mind, I was thinking the conditional would always work with a single widget - that widget however could be a container itself, and handle its own layout.

I think one nice to have thing with the single widget approach is that if we do eventually add support for conditional models, it would use the exact same model.

Yeah, I had the same thoughts but actually stopped and stashed the new conditional-container-widget.model.ts and conditional-container-widget-renderer.component.ts I was going with to get feedback on this minimal approach first.

Is that the suggestion then? We want this to have its own model widget model and renderer?

That'd be my suggestion, but of course alway welcome to push back. I would expect it to be no more code than this change (or close enough) and touch fewer existing files. It's trading a new pass through renderer for the changes to the existing renderer/container widget models. I also like the idea of going with simply conditional.model.ts- We can implement that by associating a renderer right now, but once we build out model support any JSON using this for widgets remains identical (because a widget is jut a model with an associated renderer)

…tional-container-widget

aaron-steinfeld · 2020-11-12T05:42:32Z

projects/dashboards/src/widgets/conditional/conditional.model.ts

+
+  @ModelProperty({
+    key: 'false',
+    type: PLAIN_OBJECT_PROPERTY.type,


ModelTemplatePropertyType.TYPE

aaron-steinfeld · 2020-11-12T05:44:24Z

projects/dashboards/src/widgets/conditional/conditional.model.ts

+  }
+
+  public getModel(): Observable<ModelJson> {
+    return this.getData().pipe(map(result => (result ? this.true : this.false)));


Based on how we're using this in the renderer, we should hydrate this modelJson, and return a real model - something like this.api.createChild(result ? this.true : this.false)

aaron-steinfeld · 2020-11-12T05:45:25Z

projects/dashboards/src/widgets/conditional/conditional-widget-renderer.component.ts

+  styleUrls: ['./conditional-widget-renderer.component.scss'],
+  changeDetection: ChangeDetectionStrategy.OnPush,
+  template: `
+    <div class="conditional-widget">


do we need the div? the instantiated widget should have its own container

aaron-steinfeld · 2020-11-12T05:45:44Z

projects/dashboards/src/widgets/conditional/conditional-widget-renderer.component.ts

+  `
+})
+export class ConditionalWidgetRendererComponent extends WidgetRenderer<ConditionalModel> {
+  @ViewChild('containerContent', { read: ViewContainerRef, static: true })


Doesn't appear used

…tional-container-widget

aaron-steinfeld · 2020-11-13T02:49:02Z

projects/dashboards/src/widgets/conditional/conditional-widget-renderer.component.ts

-      <ng-container [hdaDashboardModel]="this.getModel() | async"> </ng-container>
-    </div>
-  `
+  template: ` <ng-container [hdaDashboardModel]="this.getChildModel() | async"> </ng-container> `


by virtue of making this our fetch data implementation, this is already available as this.data$ - we could inline line 16 into fetchData.

I'll fix this locally. Thanks.

aaron-steinfeld · 2020-11-13T02:49:05Z

projects/dashboards/src/widgets/conditional/conditional-widget-renderer.component.ts

-  public getModel(): Observable<ModelJson> {
-    return this.model.getModel().pipe(takeUntil(this.destroyed$));
+  public getChildModel(): Observable<object> {
+    return this.model.getChildModel().pipe(takeUntil(this.destroyed$));


nit: this takeUntil isn't a problem but not necessary ( we used to use these a lot more if you were looking at old code - now we try instead where possible to push the subscriptions into the DOM which will auto unsubscribe.

Will fix locally. Thanks.

feat: conditional container widget

7fd2b1f

jake-bassett requested a review from a team as a code owner November 11, 2020 18:32

anandtiwary reviewed Nov 11, 2020

View reviewed changes

aaron-steinfeld reviewed Nov 11, 2020

View reviewed changes

jake-bassett added 2 commits November 11, 2020 17:18

feat: conditional widget

7b32858

Merge branch 'main' of github.com:hypertrace/hypertrace-ui into condi…

4df22db

…tional-container-widget

aaron-steinfeld reviewed Nov 12, 2020

View reviewed changes

jake-bassett added 3 commits November 12, 2020 10:45

fix: pr comments

97d893a

Merge branch 'main' of github.com:hypertrace/hypertrace-ui into condi…

ff753d3

…tional-container-widget

fix: import order

556277d

aaron-steinfeld approved these changes Nov 13, 2020

View reviewed changes

Merge branch 'main' into conditional-container-widget

de81fd6

jake-bassett merged commit 8276198 into main Nov 13, 2020

jake-bassett deleted the conditional-container-widget branch November 13, 2020 17:25

snyk-bot mentioned this pull request Mar 19, 2021

[Snyk] Upgrade graphql-tag from 2.12.0 to 2.12.1 #705

Merged

feat: conditional container widget #348

feat: conditional container widget #348

Uh oh!

Conversation

jake-bassett commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

codecov bot commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jake-bassett Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jake-bassett commented Nov 11, 2020 •

edited

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

jake-bassett Nov 11, 2020 •

edited

Loading