Skip to content

fix: fix misc issues with gauge and donuts #950

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 29, 2021
Merged

fix: fix misc issues with gauge and donuts #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fb107f6
fix: fix misc issues with gauge and donuts
anandtiwary Jun 28, 2021
6f75ebc
refactor: remove unused code
anandtiwary Jun 28, 2021
6ebff07
refactor: addressing self review
anandtiwary Jun 28, 2021
3ae143c
refactor: addressing self review
anandtiwary Jun 28, 2021
caada59
Merge branch 'dashboard-misc-changes' of github.com:hypertrace/hypert…
anandtiwary Jun 28, 2021
b1a25ed
refactor: fixing style
anandtiwary Jun 28, 2021
c585721
Merge branch 'main' into dashboard-misc-changes
anandtiwary Jun 28, 2021
6f779ec
refactor: addressing revew commits
anandtiwary Jun 29, 2021
0135c4d
Merge branch 'dashboard-misc-changes' of github.com:hypertrace/hypert…
anandtiwary Jun 29, 2021
9eb1fda
refactor: fixing lint errors
anandtiwary Jun 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ browserslist
LICENSE*
conf/
coverage/
test-results/
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! Been meaning to do this for... months.

4 changes: 2 additions & 2 deletions projects/components/src/titled-content/titled-content.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
}

.title {
@include widget-title($gray-9);
@include overline($gray-9);
margin-top: 16px;
}
}
Expand All @@ -40,7 +40,7 @@

.footer {
.title {
@include widget-title($gray-4);
@include overline($gray-4);
margin-top: 8px;
}
}
Expand Down
2 changes: 2 additions & 0 deletions projects/observability/src/public-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ export { LegendPosition } from './shared/components/legend/legend.component';

// Donut
export * from './shared/components/donut/donut';
export * from './shared/components/donut/donut.component';
export * from './shared/components/donut/donut.module';

// Pages
export * from './pages/apis/backends/backend-list.module';
Expand Down
15 changes: 2 additions & 13 deletions projects/observability/src/shared/components/donut/donut-builder.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ export class DonutBuilderService extends D3VisualizationBuilderService<
private static readonly DONUT_VALUE_CLASS: string = 'donut-value';
private static readonly DONUT_ARC_CLASS: string = 'donut-arc';

private static readonly DONUT_PADDING_PX: number = 60;
private static readonly LEGEND_SIZE_MULTIPLE: number = 1.5;
private static readonly DONUT_PADDING_PX: number = 10;

public constructor(
d3: D3UtilService,
Expand Down Expand Up @@ -128,17 +127,7 @@ export class DonutBuilderService extends D3VisualizationBuilderService<
protected decorateDimensions(calculatedDimensions: ChartDimensions): DonutDimensions {
let diameter = Math.min(calculatedDimensions.visualizationWidth, calculatedDimensions.visualizationHeight);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skjindal93 @aaron-steinfeld Do you guys remember why we need this logic? It seems to reduce the size of donut by a lot. It creates problem with smaller container size.

After I removed it, I didn't notice any issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, we wanted the smaller dimension to decide the diameter for the donut. If the screen has more width than the height, the height determines the diameter and vice versa

If there is a way to automatically fit in the donut, irrespective of different screen sizes, then that would be perfect

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this logic was for sizing, based on the original donut mocks way back. The comments explain it, but it's mostly about legend, padding and fitting a square visualization into potentially rectangular bounding boxes like @skjindal93 mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, so with the new changes, the Donut is still working fine. It is however making a better use of the space and appearing bigger. Apart from that, I am not seeing any failure in terms of rendering. We might want to reduce the size of the dashboard containers though so that it can match previous size.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little hesitant about this change, mainly wrt the legend. Two things are being changed here.

  1. We're not assigning legend width (why not? are we not using a legend?). Does legend still work if rendered below? to the side?
  2. We're not using the configured padding. This is fine if that's the intended visual change, but if so, any supporting code like that constant should be removed. That said, I'd guess that would show up as the legend sitting right next to the chart with 0 gap, which will look broken too.

The logic looks like it's doing these things regardless of legend position (If I vaguely recall, I think when written we always showed right legends) so that should be fixed, but this reads to me like we changed an assumption of a legend into an assumption of no legend, which is equally wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are already using Flex in container. So css is automatically assigning them correct width and position. The padding also appears to be fine visually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me share the screenshot in slack


// Save available width before reducing width by adding padding
const chartWidth = calculatedDimensions.visualizationWidth;

// Legend takes up to width of donut or remaining available space (This needs more work to look good in all cases)
calculatedDimensions.legendWidth = Math.min(
diameter * DonutBuilderService.LEGEND_SIZE_MULTIPLE,
chartWidth - diameter
);

// Reduce diameter by padding amount (don't need to do this with legend since it provides its own padding)
diameter -= DonutBuilderService.DONUT_PADDING_PX * 2;
diameter -= DonutBuilderService.DONUT_PADDING_PX;

// Reduce visualization area to diameter
calculatedDimensions.visualizationWidth = diameter;
Expand Down
1 change: 0 additions & 1 deletion projects/observability/src/shared/components/donut/donut.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export class DonutComponent implements OnChanges, OnDestroy, AfterViewInit {
public ngOnChanges(): void {
this.draw();
}

public ngOnDestroy(): void {
this.donut && this.donut.destroy();
}
Expand Down
6 changes: 5 additions & 1 deletion projects/observability/src/shared/components/gauge-list/gauge-list.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,15 @@
width: 100%;
display: grid;
margin-top: 16px;
grid-template-columns: minmax(60px, auto) minmax(80px, 160px) max-content;
grid-template-columns: minmax(60px, 1fr) max-content;
grid-auto-rows: 20px;
column-gap: 16px;
align-items: center;

&.with-label {
grid-template-columns: minmax(60px, auto) minmax(60px, 1fr) max-content;
}

.border-top {
grid-column-start: 1;
grid-column-end: 4;
Expand Down
13 changes: 10 additions & 3 deletions projects/observability/src/shared/components/gauge-list/gauge-list.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,20 @@ import { maxBy } from 'lodash-es';
styleUrls: ['./gauge-list.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<div class="gauge-list">
<div class="gauge-list" [ngClass]="{ 'with-label': this.showLabels }">
<ng-container *ngFor="let item of this.itemOptions">
<div class="border-top"></div>
<div class="border-top" *ngIf="this.showItemBorders"></div>
<div
class="label"
*ngIf="this.showLabels"
[htTooltip]="item.label"
(click)="this.onItemClick(item)"
[ngClass]="{ clickable: this.itemClickable }"
[ngStyle]="{ color: item.color }"
>
{{ item.label }}
</div>
<div class="progress">
<div class="progress" [htTooltip]="item.label">
<div class="progress-value" [ngStyle]="{ width: item.width, backgroundColor: item.color }"></div>
</div>
<div class="value">
Expand All @@ -39,6 +40,12 @@ export class GaugeListComponent<T extends GaugeItem = GaugeItem> implements OnCh
@Input()
public determineColor?: (colorKey: string) => string;

@Input()
public showLabels: boolean = true;

@Input()
public showItemBorders: boolean = true;

@Output()
public readonly itemClick: EventEmitter<T> = new EventEmitter();

Expand Down
28 changes: 24 additions & 4 deletions projects/observability/src/shared/components/utils/d3/d3-visualization-builder.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,15 +220,20 @@ export abstract class D3VisualizationBuilderService<
// tslint:disable-next-line: no-null-keyword
visualizationSelection.style('display', null);

const isLegendVisible = this.isLegendVisible(config);
const isTopOrBottomLegend = this.isTopOrBottomLegend(config);
const isSideLegend = config.legend === LegendPosition.Right;
const legendWidth = isSideLegend
let legendWidth = isLegendVisible
? 0
: isSideLegend
? Math.min(legendRect.width, this.getMaxLegendWidth())
: isTopOrBottomLegend
? outerRect.width
: 0;

const legendHeight = isTopOrBottomLegend
let legendHeight = isLegendVisible
? 0
: isTopOrBottomLegend
? Math.min(legendRect.height, this.getMaxLegendHeight())
: isSideLegend
? outerRect.height
Expand All @@ -237,8 +242,19 @@ export abstract class D3VisualizationBuilderService<
const legendWidthOffset = isSideLegend ? legendWidth : 0;
const legendHeightOffset = isTopOrBottomLegend ? legendHeight : 0;

const vizWidth = outerRect.width - legendWidthOffset;
const vizHeight = outerRect.height - legendHeightOffset;
let vizWidth = outerRect.width - legendWidthOffset;
let vizHeight = outerRect.height - legendHeightOffset;

// Hide Legend if less space is available for the viz
if (vizWidth <= legendWidthOffset || legendWidth <= 60) {
vizWidth = outerRect.width;
legendWidth = 0;
}

if (vizHeight <= legendHeightOffset || legendHeight <= 12) {
vizHeight = outerRect.height;
legendHeight = 0;
}

return this.decorateDimensions({
visualizationWidth: vizWidth,
Expand All @@ -248,6 +264,10 @@ export abstract class D3VisualizationBuilderService<
});
}

private isLegendVisible(config: ChartConfig): boolean {
return config.legend === LegendPosition.None;
}

private isTopOrBottomLegend(config: ChartConfig): boolean {
switch (config.legend) {
case LegendPosition.Bottom:
Expand Down
13 changes: 7 additions & 6 deletions projects/observability/src/shared/components/utils/d3/d3-visualization.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
.chart-container {
flex: 1 1 auto;
position: absolute; // We want parent to ignore size to detect resizes
display: flex;
align-items: center;
Expand All @@ -7,24 +8,24 @@
&.row {
flex-direction: row;
.chart-legend-container {
padding-left: 48px;
padding-right: 12px;
margin-left: 12px;
margin-right: 12px;
}
}

&.column {
flex-direction: column;
.chart-legend-container {
padding-top: 24px;
padding-bottom: 12px;
margin-top: 24px;
margin-bottom: 12px;
}
}

&.column-reverse {
flex-direction: column-reverse;
.chart-legend-container {
padding-bottom: 24px;
padding-top: 12px;
margin-bottom: 24px;
margin-top: 12px;
}
}

Expand Down