feat: adding a new gauge widget #337
Conversation
- Using angular svg binding - Using d3 shape to build arc path string - Using resize observer to react on dom dimension changes
Codecov Report
@@ Coverage Diff @@
## main #337 +/- ##
==========================================
+ Coverage 86.20% 86.24% +0.03%
==========================================
Files 721 723 +2
Lines 14683 14733 +50
Branches 1827 1832 +5
==========================================
+ Hits 12658 12706 +48
- Misses 1993 1995 +2
Partials 32 32
Continue to review full report at Codecov.
|
package.json
Outdated
| @@ -60,6 +60,7 @@ | |||
| "graphql": "^15.3.0", | |||
| "graphql-tag": "^2.11.0", | |||
| "lodash-es": "^4.17.15", | |||
| "resize-observer-polyfill": "^1.5.1", | |||
There was a problem hiding this comment.
We already have our own implementation of this which we use on all other visualizations. I can't remember the exact reason why we didn't use something like resize observer, but I do remember looking at it. Example usage from cartesian:
<div #chartContainer class="fill-container" (htLayoutChange)="this.redraw()"></div>There was a problem hiding this comment.
htLayoutChange is triggered for the entire window/dashboard. We can use it, but i strongly feel we should use the resizeObserver from inside this component. It would get triggered even if window/layout size doesn't change. Like if we have gauge and another component inside a flex container, and the other component is removed with NgIf, layout change wouldn't get triggered.
There was a problem hiding this comment.
htLayoutChange uses the window as its root, but is hierarchical - each new layout change directive creates a new scope. Even if we eventually switch to a lib (don't think it's needed personally), that should be wrapped in this service for simplified use.
There was a problem hiding this comment.
Right now we have wrapped it up into an rxjs observable easily. Do we still need a service ? If yes, what would we gain?
There was a problem hiding this comment.
So trying to page back to why we implemented it ourselves, I think other than the feature not having widespread support, the issue is that the way we often have implemented layouts (particularly with d3 stuff), a resize wouldn't trigger the way we'd want. For example, we may have done a measurement to set the svg's width, so when other parts of the dom are hidden, this component hasn't actually changed size - it's already explicitly sized. That also would applied to a parent div which got its size based on its child content.
There was a problem hiding this comment.
I tried to use LayoutChangeService and replaced it in the design I have above. The first time It tries to render (Default, without layout change), the root element gets a wrong width. This was not happening with the resize observer we used before.
Unclear what you mean - how would adding a (non-structural) directive change the width? Can you post a separate PR where you try the layout change directive? This is the method we use on every other svg, so if there are issues, we should raise them.
Is this the problem you were mentioning above?
The problem I'm referencing above is that a responsive svg scales evenly for all elements, text included. That's not behavior we want with our visualizations.
There was a problem hiding this comment.
#339 here you go . The problem is the dom first has a larger width. So it rendering origin is off. Then during the next trigger, it gets the correct width. Wondering why it is happening. With Resize observable, this doesn't happen.
There was a problem hiding this comment.
My guess is that the same issue that appears in both - you're building the measuring observable in the constructor, which is too early to measure as the dom hasn't been laid out yet. In this form, you're debouncing the resize observable which means the first draw doesn't actually happen at that moment, and is likely delayed until after drawing. Try assigning it in afterViewInit instead.
There was a problem hiding this comment.
Right. ViewInit was somehow adding a lot of delay I have tried that already. I am using debounce to fix this.
There was a problem hiding this comment.
Updated this PR. Disregard the other one
| private buildOrigin(boundingBox: ClientRect, radius: number): Point { | ||
| return { | ||
| x: boundingBox.width / 2, | ||
| y: boundingBox.height / 2 + radius / 2 |
There was a problem hiding this comment.
This seems strange, assuming a square bounding box, this makes the origin 3/4 of the way down?
There was a problem hiding this comment.
Yeah. Because we want the center of the gauge to be at the center of the container. Hence the offset by radius/2.
There was a problem hiding this comment.
So the radius is basically the height of the gauge right? Which means a gauge takes up half the height of its container (why not the full height with some padding) ?
And am I reading this logic right - it's putting the origin y at the bottom of the gauge (not the middle of the gauge), and the origin x in the middle of the gauge (which is always the center of the bounding box).
|
@aaron-steinfeld Btw how did you find the angular, svg and d3 mix-up? I think we could use this approach for some other basic svg charts. |
In general I like it - as long as we resolve that concern about text sizing and relative vs explicit dimensions. |
| return { | ||
| x: boundingBox.width / 2, | ||
| y: boundingBox.height / 2 + radius / 2 | ||
| y: boundingBox.height - GaugeComponent.GAUGE_AXIS_PADDING |
There was a problem hiding this comment.
In the case where our height is greater than our width, I believe this is is incorrect -
Take, for example a width of 300 and height of 600 - we calculate a radius of 300 - 30 = 270. Our origin here would be at (150, 570) - instead, we'd likely want it centered which would be more like (150, 450) - the padding not being needed because it's already accounted for in the centering. I think something like min(height - padding, height / 2 + min(height, width) / 2) => min(570, 450)
|
@aaron-steinfeld I think I addressed all the comments. Let me know if I miss anything |
|
So I started playing with this locally to investigate whether the debounce was needed and noticed issues with the radius. Some of these things are just style, but there's a measurement issue in here too and I was able to remove the debounce/observables: diff --git a/projects/components/src/gauge/gauge.component.ts b/projects/components/src/gauge/gauge.component.ts
index 1f2cd49b..093268f2 100644
--- a/projects/components/src/gauge/gauge.component.ts
+++ b/projects/components/src/gauge/gauge.component.ts
@@ -1,18 +1,11 @@
import { ChangeDetectionStrategy, Component, ElementRef, Input, OnChanges } from '@angular/core';
import { Color, Point } from '@hypertrace/common';
import { Arc, arc, DefaultArcObject } from 'd3-shape';
-import { BehaviorSubject, combineLatest, Observable, Subject } from 'rxjs';
-import { debounceTime, map } from 'rxjs/operators';
@Component({
selector: 'ht-gauge',
template: `
- <svg
- #chartContainer
- class="gauge"
- (htLayoutChange)="this.onLayoutChange()"
- *ngIf="this.gaugeRendererData$ | async as rendererData"
- >
+ <svg #chartContainer class="gauge" (htLayoutChange)="this.onLayoutChange()" *ngIf="this.rendererData">
<g attr.transform="translate({{ rendererData.origin.x }}, {{ rendererData.origin.y }})">
<path class="gauge-ring" [attr.d]="rendererData.backgroundArc" />
<g
@@ -50,41 +43,32 @@ export class GaugeComponent implements OnChanges {
@Input()
public thresholds: GaugeThreshold[] = [];
- public readonly gaugeRendererData$: Observable<GaugeSvgRendererData>;
+ public rendererData?: GaugeSvgRendererData;
- private readonly inputDataSubject: Subject<GaugeInputData | undefined> = new BehaviorSubject<
- GaugeInputData | undefined
- >(undefined);
- private readonly inputData$: Observable<GaugeInputData | undefined> = this.inputDataSubject.asObservable();
-
- private readonly redrawSubject: Subject<true> = new BehaviorSubject(true);
- private readonly redraw$: Observable<true> = this.redrawSubject.pipe(debounceTime(100));
-
- public constructor(public readonly elementRef: ElementRef) {
- this.gaugeRendererData$ = this.buildGaugeRendererDataObservable();
- }
+ public constructor(public readonly elementRef: ElementRef) {}
public ngOnChanges(): void {
- this.emitInputData();
+ this.rendererData = this.buildRendererData();
}
public onLayoutChange(): void {
- this.redrawSubject.next(true);
+ this.rendererData = this.buildRendererData();
}
- private buildGaugeRendererDataObservable(): Observable<GaugeSvgRendererData> {
- return combineLatest([this.inputData$, this.redraw$]).pipe(
- map(([inputData]) => {
- const boundingBox = this.elementRef.nativeElement.getBoundingClientRect();
- const radius = this.buildRadius(boundingBox);
+ private buildRendererData(): GaugeSvgRendererData | undefined {
+ const inputData = this.calculateInputData();
+ if (!inputData) {
+ return undefined;
+ }
- return {
- origin: this.buildOrigin(boundingBox, radius),
- backgroundArc: this.buildBackgroundArc(radius),
- data: this.buildGaugeData(radius, inputData)
- };
- })
- );
+ const boundingBox = this.elementRef.nativeElement.getBoundingClientRect();
+ const radius = this.buildRadius(boundingBox);
+
+ return {
+ origin: this.buildOrigin(boundingBox, radius),
+ backgroundArc: this.buildBackgroundArc(radius),
+ data: this.buildGaugeData(radius, inputData)
+ };
}
private buildBackgroundArc(radius: number): string {
@@ -123,7 +107,7 @@ export class GaugeComponent implements OnChanges {
private buildRadius(boundingBox: ClientRect): number {
return Math.min(
boundingBox.height - GaugeComponent.GAUGE_AXIS_PADDING,
- boundingBox.height / 2 + Math.min(boundingBox.height, boundingBox.width) / 2
+ boundingBox.width / 2 - GaugeComponent.GAUGE_AXIS_PADDING / 2
);
}
@@ -134,22 +118,20 @@ export class GaugeComponent implements OnChanges {
};
}
- private emitInputData(): void {
- let inputData;
+ private calculateInputData(): GaugeInputData | undefined {
if (this.value !== undefined && this.maxValue !== undefined && this.maxValue > 0 && this.thresholds.length > 0) {
const currentThreshold = this.thresholds.find(
threshold => this.value! >= threshold.start && this.value! < threshold.end
);
if (currentThreshold) {
- inputData = {
+ return {
value: this.value,
maxValue: this.maxValue,
threshold: currentThreshold
};
}
}
- this.inputDataSubject.next(inputData);
}
}
|
|
This was also a good exercise because the reason for the debounce was a wider-spread issue with an easy fix. In angular, components are rendered from the bottom of the tree to the top - so a gauge would render before the application frame, which will take up some of the available space. In most places, we put our components in dashboards which are async and control the available space anyway, so it's not an issue, but we should account for the sync case too which I believe is what you were running into. I did that while testing by hiding the frame's content until after view init. Another approach could be emitting a layout change (or via resize observer too), but for both of those, we'd just be doing a second render - preventing seems better. |
|
Axis Padding is a vertical padding, I don't think we should use the same for width as well. Just noticed that Debounce is applied already in layout service. |
Yeah, I had tried this with observable approach too. Basically emitting on redrawSubject on viewInit rather than using a behavior subject. But the rendering got delayed by few seconds somehow. With your approach, was it still instantaneous? |
Can remove the padding on the width side if not applicable there, assumed it was uniform. |
So the view init in gauge alone doesn't help, because that runs before the parent's view init which can change the available space - the approach I took, it felt instantaneous because the frame waits to render it's content until the top/side are set up. |
Description
feat: adding a new gauge widget
Testing
Deployed, tested locally and added unit test to verify certain dom changes
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.
Sure