Skip to content
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.

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

fix(legend): improve last value handling #2115

Merged
merged 24 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
39718b3
fix: last time bucket for legend extra
markov00 Jul 27, 2023
e64ce9f
add example and fix last value
markov00 Sep 28, 2023
b3ad6fe
improve example
markov00 Oct 2, 2023
88c2561
remove unused prop
markov00 Oct 2, 2023
b76c7b5
revert changes to scales
markov00 Dec 1, 2023
314c7b9
Merge branch 'main' into 2023_07_27-fix_last_bucket_time
markov00 Dec 1, 2023
85e500b
move test under test-cases vrts
markov00 Dec 1, 2023
76b048d
reenable last value on linear scales
markov00 Dec 1, 2023
91272b3
use last bucket for linear last value
markov00 Dec 1, 2023
c6da82a
fix last time bucket in DST
markov00 Dec 1, 2023
2fd8d3c
handle last bucket in both time and continuous scales
markov00 Dec 4, 2023
1df6977
the consumer is responsible to pass all the data to the chart
markov00 Dec 6, 2023
944f25d
Merge branch 'main' into 2023_07_27-fix_last_bucket_time
markov00 Dec 19, 2023
c9d8bfb
remove unused function
markov00 Dec 19, 2023
f31216f
use data domain as domain extent for legend last value
markov00 Dec 19, 2023
ae660ba
revert example changes
markov00 Dec 19, 2023
4188b04
test(vrt): update screenshots [skip ci]
elastic-datavis[bot] Dec 19, 2023
c44f450
Merge branch 'main' into 2023_07_27-fix_last_bucket_time
markov00 Dec 20, 2023
4addc3b
update test screenshot [update-vrt]
markov00 Dec 21, 2023
01051a8
add screenshots [update-vrt]
markov00 Dec 21, 2023
f184e51
test(vrt): update screenshots [skip ci]
elastic-datavis[bot] Dec 21, 2023
75404f3
Merge branch 'main' into 2023_07_27-fix_last_bucket_time
markov00 Jan 8, 2024
6af36e5
fix missing axis ticks in time axis [update-vrt]
markov00 Jan 9, 2024
ed3fa57
test(vrt): update screenshots [skip ci]
elastic-datavis[bot] Jan 9, 2024
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
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
27 changes: 27 additions & 0 deletions e2e/tests/test_cases_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,31 @@ test.describe('Test cases stories', () => {
);
});
});

test.describe('Legend last value should be aligned across areas and bars', () => {
test('data domain', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--domain-edges&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-custom domain=&knob-start time=0&knob-end time=19&knob-subtract 1ms=',
{ screenshotSelector: '#story-root' },
);
});
test('custom domain', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--domain-edges&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-custom domain=true&knob-start time=0&knob-end time=19&knob-subtract 1ms=',
{ screenshotSelector: '#story-root' },
);
});
test('custom -1ms', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--domain-edges&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-custom domain=true&knob-start time=0&knob-end time=19&knob-subtract 1ms=true',
{ screenshotSelector: '#story-root' },
);
});
test('custom empty', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--domain-edges&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-custom domain=true&knob-end time=10.2&knob-start time=0&knob-subtract 1ms=true',
{ screenshotSelector: '#story-root' },
);
});
});
});
1 change: 1 addition & 0 deletions packages/charts/src/chart_types/xy_chart/domains/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export type XDomain = Pick<LogScaleOptions, 'logBase'> & {
/** the configured timezone in the specs or the fallback to the browser local timezone */
timeZone: string;
domain: OrdinalDomain | ContinuousDomain;
dataDomain: OrdinalDomain | ContinuousDomain;
desiredTickCount: number;
};

Expand Down
22 changes: 13 additions & 9 deletions packages/charts/src/chart_types/xy_chart/domains/x_domain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,20 @@ export function mergeXDomain(
locale: string,
fallbackScale?: XScaleType,
): XDomain {
let seriesXComputedDomains;
let domain;
let dataDomain;
let minInterval = 0;

if (type === ScaleType.Ordinal || fallbackScale === ScaleType.Ordinal) {
if (type !== ScaleType.Ordinal) {
Logger.warn(`Each X value in a ${type} x scale needs be be a number. Using ordinal x scale as fallback.`);
}

seriesXComputedDomains = computeOrdinalDataDomain([...xValues], false, true, locale);
dataDomain = computeOrdinalDataDomain([...xValues], false, true, locale);
domain = dataDomain;
if (customDomain) {
if (Array.isArray(customDomain)) {
seriesXComputedDomains = [...customDomain];
domain = [...customDomain];
} else {
if (fallbackScale === ScaleType.Ordinal) {
Logger.warn(`xDomain ignored for fallback ordinal scale. Options to resolve:
Expand All @@ -54,37 +56,38 @@ export function mergeXDomain(
}
} else {
const domainOptions = { min: NaN, max: NaN, fit: true };
seriesXComputedDomains = computeContinuousDataDomain([...xValues] as number[], type, domainOptions);
dataDomain = computeContinuousDataDomain([...xValues] as number[], type, domainOptions);
domain = dataDomain;
let customMinInterval: undefined | number;

if (customDomain) {
if (Array.isArray(customDomain)) {
Logger.warn('xDomain for continuous scale should be a DomainRange object, not an array');
} else {
customMinInterval = customDomain.minInterval;
const [computedDomainMin, computedDomainMax] = seriesXComputedDomains;
const [computedDomainMin, computedDomainMax] = domain;

if (Number.isFinite(customDomain.min) && Number.isFinite(customDomain.max)) {
if (customDomain.min > customDomain.max) {
Logger.warn('Custom xDomain is invalid: min is greater than max. Custom domain is ignored.');
} else {
seriesXComputedDomains = [customDomain.min, customDomain.max];
domain = [customDomain.min, customDomain.max];
}
} else if (Number.isFinite(customDomain.min)) {
if (customDomain.min > computedDomainMax) {
Logger.warn(
'Custom xDomain is invalid: custom min is greater than computed max. Custom domain is ignored.',
);
} else {
seriesXComputedDomains = [customDomain.min, computedDomainMax];
domain = [customDomain.min, computedDomainMax];
}
} else if (Number.isFinite(customDomain.max)) {
if (computedDomainMin > customDomain.max) {
Logger.warn(
'Custom xDomain is invalid: computed min is greater than custom max. Custom domain is ignored.',
);
} else {
seriesXComputedDomains = [computedDomainMin, customDomain.max];
domain = [computedDomainMin, customDomain.max];
}
}
}
Expand All @@ -97,7 +100,8 @@ export function mergeXDomain(
type: fallbackScale ?? type,
nice,
isBandScale,
domain: seriesXComputedDomains,
domain,
dataDomain,
minInterval,
timeZone: getValidatedTimeZone(timeZone),
logBase: customDomain && 'logBase' in customDomain ? customDomain.logBase : 10, // fixme preexisting TS workaround
Expand Down
38 changes: 2 additions & 36 deletions packages/charts/src/chart_types/xy_chart/legend/legend.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import { Store } from 'redux';

import { getLegendExtra } from './legend';
import { ChartType } from '../..';
import { MockGlobalSpec, MockSeriesSpec } from '../../../mocks/specs/specs';
import { MockStore } from '../../../mocks/store/store';
Expand All @@ -24,9 +23,9 @@ import { getSeriesName } from '../utils/series';
import { AxisSpec, BasicSeriesSpec, SeriesType } from '../utils/specs';

const nullDisplayValue = {
formatted: null,
raw: null,
legendSizingLabel: null,
formatted: '',
legendSizingLabel: '',
};

const spec1: BasicSeriesSpec = {
Expand Down Expand Up @@ -402,37 +401,4 @@ describe('Legends', () => {
name = getSeriesName(seriesIdentifier1, false, false, specWithSplit);
expect(name).toBe('Spec 1 title');
});
it('should return correct legendSizingLabel with linear scale and showExtraLegend set to true', () => {
const formatter = (d: string | number) => `${Number(d).toFixed(2)} dogs`;
const lastValues = { y0: null, y1: 14 };
const showExtraLegend = true;
const xScaleIsLinear = ScaleType.Linear;

expect(getLegendExtra(showExtraLegend, xScaleIsLinear, formatter, 'y1', lastValues)).toMatchObject({
raw: 14,
formatted: '14.00 dogs',
legendSizingLabel: '14.00 dogs',
});
});
it('should return formatted to null with ordinal scale and showExtraLegend set to true', () => {
const formatter = (d: string | number) => `${Number(d).toFixed(2)} dogs`;
const lastValues = { y0: null, y1: 14 };

expect(getLegendExtra(true, ScaleType.Ordinal, formatter, 'y1', lastValues)).toMatchObject({
raw: 14,
formatted: null,
legendSizingLabel: '14.00 dogs',
});
});
it('should return legendSizingLabel null with showLegendExtra set to false', () => {
const formatter = (d: string | number) => `${Number(d).toFixed(2)} dogs`;
const lastValues = { y0: null, y1: 14 };
const showLegendExtra = false;

expect(getLegendExtra(showLegendExtra, ScaleType.Ordinal, formatter, 'y1', lastValues)).toMatchObject({
raw: null,
formatted: null,
legendSizingLabel: null,
});
});
});
66 changes: 32 additions & 34 deletions packages/charts/src/chart_types/xy_chart/legend/legend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,14 @@
import { Color } from '../../../common/colors';
import { LegendItem } from '../../../common/legend';
import { SeriesKey, SeriesIdentifier } from '../../../common/series_id';
import { ScaleType } from '../../../scales/constants';
import { SettingsSpec, TickFormatterOptions } from '../../../specs';
import { SettingsSpec } from '../../../specs';
import { isDefined, mergePartial } from '../../../utils/common';
import { BandedAccessorType } from '../../../utils/geometry';
import { getLegendCompareFn, SeriesCompareFn } from '../../../utils/series_sort';
import { PointStyle, Theme } from '../../../utils/themes/theme';
import { getXScaleTypeFromSpec } from '../scales/get_api_scales';
import { XDomain } from '../domains/types';
import { LegendValue, getLegendValue } from '../state/utils/get_last_value';
import { getAxesSpecForSpecId, getSpecsById } from '../state/utils/spec';
import { LastValues } from '../state/utils/types';
import { Y0_ACCESSOR_POSTFIX, Y1_ACCESSOR_POSTFIX } from '../tooltip/tooltip';
import { defaultTickFormatter } from '../utils/axis_utils';
import { defaultXYLegendSeriesSort } from '../utils/default_series_sort_fn';
Expand All @@ -34,6 +33,7 @@ import {
AxisSpec,
BasicSeriesSpec,
Postfixes,
StackMode,
isAreaSeriesSpec,
isBarSeriesSpec,
isBubbleSeriesSpec,
Expand Down Expand Up @@ -61,27 +61,6 @@ function getBandedLegendItemLabel(name: string, yAccessor: BandedAccessorType, p
: `${name}${postfixes.y0AccessorFormat}`;
}

/** @internal */
export function getLegendExtra(
showLegendExtra: boolean,
xScaleType: ScaleType,
formatter: (value: any, options?: TickFormatterOptions | undefined) => string,
key: keyof LastValues,
lastValue?: LastValues,
): LegendItem['defaultExtra'] {
if (showLegendExtra) {
const rawValue = (lastValue && lastValue[key]) ?? null;
const formattedValue = rawValue !== null ? formatter(rawValue) : null;

return {
raw: rawValue !== null ? rawValue : null,
formatted: xScaleType === ScaleType.Ordinal ? null : formattedValue,
legendSizingLabel: formattedValue,
};
}
return { raw: null, formatted: null, legendSizingLabel: null };
}

/** @internal */
function getPointStyle(spec: BasicSeriesSpec, theme: Theme): PointStyle | undefined {
if (isBubbleSeriesSpec(spec)) {
Expand All @@ -95,8 +74,8 @@ function getPointStyle(spec: BasicSeriesSpec, theme: Theme): PointStyle | undefi

/** @internal */
export function computeLegend(
xDomain: XDomain,
dataSeries: DataSeries[],
lastValues: Map<SeriesKey, LastValues>,
seriesColors: Map<SeriesKey, Color>,
specs: BasicSeriesSpec[],
axesSpecs: AxisSpec[],
Expand All @@ -108,10 +87,11 @@ export function computeLegend(
const legendItems: LegendItem[] = [];
const defaultColor = theme.colors.defaultVizColor;

const legendValueMode = LegendValue.LastValue;

dataSeries.forEach((series) => {
const { specId, yAccessor } = series;
const banded = isBandedSpec(series.spec);
const key = getSeriesKey(series, series.groupId);
const spec = getSpecsById<BasicSeriesSpec>(specs, specId);
const dataSeriesKey = getSeriesKey(
{
Expand All @@ -129,33 +109,47 @@ export function computeLegend(
if (name === '' || !spec) return;

const postFixes = getPostfix(spec);
const labelY1 = banded ? getBandedLegendItemLabel(name, BandedAccessorType.Y1, postFixes) : name;

// Use this to get axis spec w/ tick formatter
const { yAxis } = getAxesSpecForSpecId(axesSpecs, spec.groupId, settingsSpec.rotation);
const formatter = spec.tickFormat ?? yAxis?.tickFormat ?? defaultTickFormatter;
const { hideInLegend } = spec;

const lastValue = lastValues.get(key);
const seriesIdentifier = getSeriesIdentifierFromDataSeries(series);
const xScaleType = getXScaleTypeFromSpec(spec.xScaleType);

const pointStyle = getPointStyle(spec, theme);

const itemValue = getLegendValue(series, xDomain, legendValueMode, (d) => {
return series.stackMode === StackMode.Percentage
? d.y1 === null || d.y0 === null
? null
: d.y1 - d.y0
: d.initialY1;
});
const formattedItemValue = itemValue !== null ? formatter(itemValue) : '';

legendItems.push({
color,
label: labelY1,
label: banded ? getBandedLegendItemLabel(name, BandedAccessorType.Y1, postFixes) : name,
seriesIdentifiers: [seriesIdentifier],
childId: BandedAccessorType.Y1,
isSeriesHidden,
isItemHidden: hideInLegend,
isToggleable: true,
defaultExtra: getLegendExtra(settingsSpec.showLegendExtra, xScaleType, formatter, 'y1', lastValue),
defaultExtra: {
raw: itemValue,
formatted: formattedItemValue,
legendSizingLabel: formattedItemValue,
},
path: [{ index: 0, value: seriesIdentifier.key }],
keys: [specId, spec.groupId, yAccessor, ...series.splitAccessors.values()],
pointStyle,
});
if (banded) {
const bandedItemValue = getLegendValue(series, xDomain, legendValueMode, (d) => {
return series.stackMode === StackMode.Percentage ? d.y0 : d.initialY0;
});
const bandedFormattedItemValue = bandedItemValue !== null ? formatter(bandedItemValue) : '';

const labelY0 = getBandedLegendItemLabel(name, BandedAccessorType.Y0, postFixes);
legendItems.push({
color,
Expand All @@ -165,7 +159,11 @@ export function computeLegend(
isSeriesHidden,
isItemHidden: hideInLegend,
isToggleable: true,
defaultExtra: getLegendExtra(settingsSpec.showLegendExtra, xScaleType, formatter, 'y0', lastValue),
defaultExtra: {
raw: bandedItemValue,
formatted: bandedFormattedItemValue,
legendSizingLabel: bandedFormattedItemValue,
},
path: [{ index: 0, value: seriesIdentifier.key }],
keys: [specId, spec.groupId, yAccessor, ...series.splitAccessors.values()],
pointStyle,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { getDeselectedSeriesSelector } from '../../../../state/selectors/get_des
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_spec';
import { computeLegend } from '../../legend/legend';
import { DataSeries } from '../../utils/series';
import { getLastValues } from '../utils/get_last_value';

/** @internal */
export const computeLegendSelector = createCustomCachedSelector(
Expand All @@ -42,8 +41,8 @@ export const computeLegendSelector = createCustomCachedSelector(
siDataSeriesMap: Record<string, DataSeries>,
): LegendItem[] => {
return computeLegend(
xDomain,
formattedDataSeries,
getLastValues(formattedDataSeries, xDomain),
seriesColors,
seriesSpecs,
axesSpecs,
Expand Down
Loading