Skip to content

feat: toggle able legend for cartesian chart #1270

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
itssharmasandeep merged 14 commits into from
Dec 2, 2021
Merged

feat: toggle able legend for cartesian chart #1270

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a3be401
feat: toggle able legend for cartesian chart
itssharmasandeep Nov 18, 2021
1fe78fe
fix: add test cases
itssharmasandeep Nov 18, 2021
1fbd6c9
Merge remote-tracking branch 'origin/main' into cartesian-toggle-legend
itssharmasandeep Nov 19, 2021
b53fe40
fix: addressing review comments
itssharmasandeep Nov 19, 2021
747556e
fix: addressing review comments
itssharmasandeep Nov 22, 2021
09513b3
fix: addressing review comments
itssharmasandeep Nov 22, 2021
1781dee
Merge remote-tracking branch 'origin/main' into cartesian-toggle-legend
itssharmasandeep Nov 25, 2021
805d7e1
fix: some style changes
itssharmasandeep Nov 26, 2021
c9df216
Merge remote-tracking branch 'origin/main' into cartesian-toggle-legend
itssharmasandeep Nov 30, 2021
9f5f84b
Merge remote-tracking branch 'origin/main' into cartesian-toggle-legend
itssharmasandeep Nov 30, 2021
d9be399
fix: addressing review comments
itssharmasandeep Dec 1, 2021
a9b166b
fix: suggested changes
itssharmasandeep Dec 1, 2021
c78844b
fix: lint fixes
itssharmasandeep Dec 1, 2021
c2f177b
fix: addressing review comments
itssharmasandeep Dec 2, 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
8 changes: 4 additions & 4 deletions projects/observability/src/shared/components/cartesian/d3/chart/cartesian-chart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export class DefaultCartesianChart<TData> implements CartesianChart<TData> {
protected chartBackgroundSvgElement?: SVGSVGElement;
protected dataElement?: ContainerElement;
protected mouseEventContainer?: SVGSVGElement;
protected legend?: CartesianLegend;
protected legend?: CartesianLegend<TData>;
protected tooltip?: ChartTooltipRef<TData>;
protected allSeriesData: CartesianData<TData, Series<TData>>[] = [];
protected allCartesianData: CartesianData<TData, Series<TData> | Band<TData>>[] = [];
Expand Down Expand Up @@ -368,20 +368,20 @@ export class DefaultCartesianChart<TData> implements CartesianChart<TData> {
private drawLegend(): void {
if (this.chartContainerElement) {
if (this.legendPosition !== undefined && this.legendPosition !== LegendPosition.None) {
this.legend = new CartesianLegend(
this.legend = new CartesianLegend<TData>(
this.activeSeries,
this.injector,
this.intervalData,
this.seriesSummaries
).draw(this.chartContainerElement, this.legendPosition);
this.activeSeriesSubscription?.unsubscribe();
this.activeSeriesSubscription = this.legend.activeSeries$.subscribe(activeSeries => {
this.activeSeries = activeSeries as Series<TData>[];
this.activeSeries = activeSeries;
this.redrawVisualization();
});
} else {
// The legend also contains the interval selector, so even without a legend we need to create an element for that
this.legend = new CartesianLegend([], this.injector, this.intervalData, this.seriesSummaries).draw(
this.legend = new CartesianLegend<TData>([], this.injector, this.intervalData, this.seriesSummaries).draw(
this.chartContainerElement,
LegendPosition.None
);
Expand Down
66 changes: 30 additions & 36 deletions projects/observability/src/shared/components/cartesian/d3/legend/cartesian-legend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { ComponentRef, Injector } from '@angular/core';
import { Color, DynamicComponentService } from '@hypertrace/common';
import { ContainerElement, EnterElement, select, Selection } from 'd3-selection';
import { Observable, of, Subject } from 'rxjs';
import { startWith, switchMap } from 'rxjs/operators';
import { Observable, Subject } from 'rxjs';
import { distinctUntilChanged, startWith } from 'rxjs/operators';
import { LegendPosition } from '../../../legend/legend.component';
import { Series, Summary } from '../../chart';
import {
Expand All @@ -12,36 +12,33 @@ import {
} from './cartesian-interval-control.component';
import { CartesianSummaryComponent, SUMMARIES_DATA } from './cartesian-summary.component';

export class CartesianLegend {
export class CartesianLegend<TData> {
private static readonly CSS_CLASS: string = 'legend';
private static readonly RESET_CSS_CLASS: string = 'reset';
private static readonly DEFAULT_CSS_CLASS: string = 'default';
private static readonly ACTIVE_CSS_CLASS: string = 'active';
private static readonly INACTIVE_CSS_CLASS: string = 'inactive';
public static readonly CSS_SELECTOR: string = `.${CartesianLegend.CSS_CLASS}`;

public readonly activeSeries$: Observable<Series<{}>[]>;
private readonly activeSeriesSubject: Subject<void> = new Subject();
private readonly initialSeries: Series<{}>[];
public readonly activeSeries$: Observable<Series<TData>[]>;
private readonly activeSeriesSubject: Subject<Series<TData>[]> = new Subject();
private readonly initialSeries: Series<TData>[];

private isSelectionModeOn: boolean = false;
private legendElement?: HTMLDivElement;
private activeSeries: Series<{}>[];
private activeSeries: Series<TData>[];
private intervalControl?: ComponentRef<unknown>;
private summaryControl?: ComponentRef<unknown>;

public constructor(
private readonly series: Series<{}>[],
private readonly series: Series<TData>[],
private readonly injector: Injector,
private readonly intervalData?: CartesianIntervalData,
private readonly summaries: Summary[] = []
) {
this.activeSeries = [...this.series];
this.initialSeries = [...this.series];
this.activeSeries$ = this.activeSeriesSubject.asObservable().pipe(
startWith(undefined),
switchMap(() => of(this.activeSeries))
);
this.activeSeries$ = this.activeSeriesSubject.asObservable().pipe(distinctUntilChanged(), startWith(this.series));
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Dec 1, 2021

Choose a reason for hiding this comment

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

This distinctUntilChanged doesn't solve our problem - see my previous comment about isEqual https://github.com/hypertrace/hypertrace-ui/pull/1270/files/d9be3992407b4436a1c84bc7b826c49942eeb888#r760277561

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is not working.
The problem I found it, distinctUntilChanged is not behaving the way we want. I tried to do this, to check where is the problem.

this.activeSeries$ = this.activeSeriesSubject.asObservable().pipe(
      tap(console.log),
      distinctUntilChanged((prev, curr) => {
        console.log(prev, curr);
        return prev.length === curr.length;
      })
    );

The problem I found was, prev is always the main series (the initial one).

I think, we can take this later as you mentioned, there are only two case where we would need this, only one series (this has already been taken care of where we disable the click behaviour), second when everything is selected and and then we do a reset. I think this is fine, It is also not very common to do, so we can skip

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Dec 2, 2021

Choose a reason for hiding this comment

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

I'd expect that on the first call, but assuming the first call returns true, it shouldn't be in the future. Something else is up. This is a bug, so we should fix it, but can be done as a followup.

}

public draw(hostElement: Element, position: LegendPosition): this {
Expand Down Expand Up @@ -75,12 +72,12 @@ export class CartesianLegend {
.append('span')
.classed(CartesianLegend.RESET_CSS_CLASS, true)
.text('Reset')
.on('click', () => this.makeSelectionModeOff());
.on('click', () => this.disableSelectionMode());

this.setResetVisibility(!this.isSelectionModeOn);
this.updateResetElementVisibility(!this.isSelectionModeOn);
}

private setResetVisibility(isHidden: boolean): void {
private updateResetElementVisibility(isHidden: boolean): void {
select(this.legendElement!).select(`span.${CartesianLegend.RESET_CSS_CLASS}`).classed('hidden', isHidden);
}

Expand Down Expand Up @@ -112,15 +109,15 @@ export class CartesianLegend {
.classed(`position-${legendPosition}`, true);
}

private drawLegendEntry(element: EnterElement): Selection<HTMLDivElement, Series<{}>, null, undefined> {
const legendEntry = select<EnterElement, Series<{}>>(element).append('div').classed('legend-entry', true);
private drawLegendEntry(element: EnterElement): Selection<HTMLDivElement, Series<TData>, null, undefined> {
const legendEntry = select<EnterElement, Series<TData>>(element).append('div').classed('legend-entry', true);

this.appendLegendSymbol(legendEntry);
legendEntry
.append('span')
.classed('legend-text', true)
.text(series => series.name)
.on('click', series => this.updateActiveSeries(series));
.on('click', series => (this.series.length > 1 ? this.updateActiveSeries(series) : undefined));

this.updateLegendClassesAndStyle();

Expand All @@ -134,7 +131,7 @@ export class CartesianLegend {
legendElementSelection
.selectAll('.legend-symbol circle')
.style('fill', series =>
!this.isThisLegendEntryActive(series as Series<{}>) ? Color.Gray3 : (series as Series<{}>).color
!this.isThisLegendEntryActive(series as Series<TData>) ? Color.Gray3 : (series as Series<TData>).color
);

// Legend entry value text
Expand All @@ -143,15 +140,15 @@ export class CartesianLegend {
.classed(CartesianLegend.DEFAULT_CSS_CLASS, !this.isSelectionModeOn)
.classed(
CartesianLegend.ACTIVE_CSS_CLASS,
series => this.isSelectionModeOn && this.isThisLegendEntryActive(series as Series<{}>)
series => this.isSelectionModeOn && this.isThisLegendEntryActive(series as Series<TData>)
)
.classed(
CartesianLegend.INACTIVE_CSS_CLASS,
series => this.isSelectionModeOn && !this.isThisLegendEntryActive(series as Series<{}>)
series => this.isSelectionModeOn && !this.isThisLegendEntryActive(series as Series<TData>)
);
}

private appendLegendSymbol(selection: Selection<HTMLDivElement, Series<{}>, null, undefined>): void {
private appendLegendSymbol(selection: Selection<HTMLDivElement, Series<TData>, null, undefined>): void {
selection
.append('svg')
.classed('legend-symbol', true)
Expand Down Expand Up @@ -194,32 +191,29 @@ export class CartesianLegend {
);
}

private makeSelectionModeOff(): void {
private disableSelectionMode(): void {
this.activeSeries = [...this.initialSeries];
this.isSelectionModeOn = false;
this.updateLegendClassesAndStyle();
this.setResetVisibility(!this.isSelectionModeOn);
this.activeSeriesSubject.next();
this.updateResetElementVisibility(!this.isSelectionModeOn);
this.activeSeriesSubject.next(this.activeSeries);
}

private updateActiveSeries(seriesEntry: Series<{}>): void {
private updateActiveSeries(seriesEntry: Series<TData>): void {
if (!this.isSelectionModeOn) {
this.activeSeries = [];
this.activeSeries.push(seriesEntry);
this.activeSeries = [seriesEntry];
this.isSelectionModeOn = true;
} else if (this.isThisLegendEntryActive(seriesEntry)) {
this.activeSeries = this.activeSeries.filter(series => series !== seriesEntry);
} else {
if (this.isThisLegendEntryActive(seriesEntry)) {
this.activeSeries = this.activeSeries.filter(series => series !== seriesEntry);
} else {
this.activeSeries.push(seriesEntry);
}
this.activeSeries.push(seriesEntry);
}
this.updateLegendClassesAndStyle();
this.setResetVisibility(!this.isSelectionModeOn);
this.activeSeriesSubject.next();
this.updateResetElementVisibility(!this.isSelectionModeOn);
this.activeSeriesSubject.next(this.activeSeries);
}

private isThisLegendEntryActive(seriesEntry: Series<{}>): boolean {
private isThisLegendEntryActive(seriesEntry: Series<TData>): boolean {
return this.activeSeries.includes(seriesEntry);
}
}