Skip to content

Commit

Permalink
fix(legend): legend sizes with ordinal data (opensearch-project#867)
Browse files Browse the repository at this point in the history
  • Loading branch information
rshen91 authored Oct 20, 2020
1 parent 3b372d8 commit 74bcbad
Show file tree
Hide file tree
Showing 94 changed files with 113 additions and 42 deletions.
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.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Binary file not shown.
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.
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.
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.
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.
6 changes: 6 additions & 0 deletions packages/osd-charts/integration/tests/interactions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,4 +295,10 @@ describe('Interactions', () => {
);
});
});
it('should size legends with ordinal x axis', async () => {
await common.expectChartWithMouseAtUrlToMatchScreenshot(
'http://localhost:9001/iframe.html?id=axes--different-tooltip-formatter',
{ left: 350, top: 130 },
);
});
});
58 changes: 50 additions & 8 deletions packages/osd-charts/src/chart_types/xy_chart/legend/legend.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ import { Position, RecursivePartial } from '../../../utils/commons';
import { AxisStyle } from '../../../utils/themes/theme';
import { SeriesCollectionValue, getSeriesName } from '../utils/series';
import { AxisSpec, BasicSeriesSpec, SeriesTypes } from '../utils/specs';
import { computeLegend } from './legend';
import { computeLegend, getLegendExtra } from './legend';

const nullDisplayValue = {
formatted: null,
raw: null,
legendSizingLabel: null,
};
const seriesCollectionValue1a = {
seriesIdentifier: {
Expand Down Expand Up @@ -129,7 +130,7 @@ describe('Legends', () => {
});
it('compute legend for a single series', () => {
seriesCollection.set('seriesCollectionValue1a', seriesCollectionValue1a);
const legend = computeLegend(seriesCollection, seriesCollectionMap, specs, 'violet', axesSpecs);
const legend = computeLegend(seriesCollection, seriesCollectionMap, specs, 'violet', axesSpecs, false);
const expected: LegendItem[] = [
{
color: 'red',
Expand All @@ -147,7 +148,7 @@ describe('Legends', () => {
it('compute legend for a single spec but with multiple series', () => {
seriesCollection.set('seriesCollectionValue1a', seriesCollectionValue1a);
seriesCollection.set('seriesCollectionValue1b', seriesCollectionValue1b);
const legend = computeLegend(seriesCollection, seriesCollectionMap, specs, 'violet', axesSpecs);
const legend = computeLegend(seriesCollection, seriesCollectionMap, specs, 'violet', axesSpecs, false);
const expected: LegendItem[] = [
{
color: 'red',
Expand Down Expand Up @@ -175,7 +176,7 @@ describe('Legends', () => {
it('compute legend for multiple specs', () => {
seriesCollection.set('seriesCollectionValue1a', seriesCollectionValue1a);
seriesCollection.set('seriesCollectionValue2a', seriesCollectionValue2a);
const legend = computeLegend(seriesCollection, seriesCollectionMap, specs, 'violet', axesSpecs);
const legend = computeLegend(seriesCollection, seriesCollectionMap, specs, 'violet', axesSpecs, false);
const expected: LegendItem[] = [
{
color: 'red',
Expand All @@ -202,13 +203,13 @@ describe('Legends', () => {
});
it('empty legend for missing spec', () => {
seriesCollection.set('seriesCollectionValue2b', seriesCollectionValue2b);
const legend = computeLegend(seriesCollection, seriesCollectionMap, specs, 'violet', axesSpecs);
const legend = computeLegend(seriesCollection, seriesCollectionMap, specs, 'violet', axesSpecs, false);
expect(legend.length).toEqual(0);
});
it('compute legend with default color for missing series color', () => {
seriesCollection.set('seriesCollectionValue1a', seriesCollectionValue1a);
const emptyColorMap = new Map<string, string>();
const legend = computeLegend(seriesCollection, emptyColorMap, specs, 'violet', axesSpecs);
const legend = computeLegend(seriesCollection, emptyColorMap, specs, 'violet', axesSpecs, false);
const expected: LegendItem[] = [
{
color: 'violet',
Expand All @@ -231,7 +232,7 @@ describe('Legends', () => {

const emptyColorMap = new Map<string, string>();

const legend = computeLegend(seriesCollection, emptyColorMap, specs, 'violet', axesSpecs);
const legend = computeLegend(seriesCollection, emptyColorMap, specs, 'violet', axesSpecs, false);

const visibility = [...legend.values()].map((item) => !item.isSeriesHidden);

Expand All @@ -246,7 +247,15 @@ describe('Legends', () => {
const emptyColorMap = new Map<string, string>();
const deselectedDataSeries = [seriesCollectionValue1a.seriesIdentifier, seriesCollectionValue1b.seriesIdentifier];

const legend = computeLegend(seriesCollection, emptyColorMap, specs, 'violet', axesSpecs, deselectedDataSeries);
const legend = computeLegend(
seriesCollection,
emptyColorMap,
specs,
'violet',
axesSpecs,
false,
deselectedDataSeries,
);

const visibility = [...legend.values()].map((item) => !item.isSeriesHidden);
expect(visibility).toEqual([false, false, true]);
Expand Down Expand Up @@ -344,4 +353,37 @@ 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,
});
});
});
41 changes: 33 additions & 8 deletions packages/osd-charts/src/chart_types/xy_chart/legend/legend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,12 @@

import { LegendItem } from '../../../commons/legend';
import { SeriesKey, SeriesIdentifier } from '../../../commons/series_id';
import { ScaleType } from '../../../scales/constants';
import { TickFormatterOptions } from '../../../specs';
import { Color } from '../../../utils/commons';
import { BandedAccessorType } from '../../../utils/geometry';
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 {
Expand Down Expand Up @@ -56,16 +59,43 @@ 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 */
export function computeLegend(
seriesCollection: Map<SeriesKey, SeriesCollectionValue>,
seriesColors: Map<SeriesKey, Color>,
specs: BasicSeriesSpec[],
defaultColor: string,
axesSpecs: AxisSpec[],
showLegendExtra: boolean,
deselectedDataSeries: SeriesIdentifier[] = [],
): LegendItem[] {
const legendItems: LegendItem[] = [];

const sortedCollection = getSortedDataSeriesColorsValuesMap(seriesCollection);

sortedCollection.forEach((series, key) => {
Expand All @@ -79,6 +109,7 @@ export function computeLegend(
if (name === '' || !spec) {
return;
}

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

Expand All @@ -95,10 +126,7 @@ export function computeLegend(
isSeriesHidden,
isItemHidden: hideInLegend,
isToggleable: true,
defaultExtra: {
raw: lastValue && lastValue.y1 !== null ? lastValue.y1 : null,
formatted: lastValue && lastValue.y1 !== null ? formatter(lastValue.y1) : null,
},
defaultExtra: getLegendExtra(showLegendExtra, spec.xScaleType, formatter, 'y1', lastValue),
});
if (banded) {
const labelY0 = getBandedLegendItemLabel(name, BandedAccessorType.Y0, postFixes);
Expand All @@ -110,10 +138,7 @@ export function computeLegend(
isSeriesHidden,
isItemHidden: hideInLegend,
isToggleable: true,
defaultExtra: {
raw: lastValue && lastValue.y0 !== null ? lastValue.y0 : null,
formatted: lastValue && lastValue.y0 !== null ? formatter(lastValue.y0) : null,
},
defaultExtra: getLegendExtra(showLegendExtra, spec.xScaleType, formatter, 'y0', lastValue),
});
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ describe('Rendering utils', () => {
defaultExtra: {
formatted: null,
raw: null,
legendSizingLabel: null,
},
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,6 @@ describe.skip('Chart Store', () => {
specId: SPEC_ID,
key: 'color1',
},
defaultExtra: {
raw: null,
formatted: 'formatted-last',
},
};

const secondLegendItem: LegendItem = {
Expand All @@ -90,10 +86,6 @@ describe.skip('Chart Store', () => {
specId: SPEC_ID,
key: 'color2',
},
defaultExtra: {
raw: null,
formatted: 'formatted-last',
},
};
beforeEach(() => {
store = null; // new ChartStore();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { LegendItem } from '../../../../commons/legend';
import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';
import { getChartThemeSelector } from '../../../../state/selectors/get_chart_theme';
import { getDeselectedSeriesSelector } from '../../../../state/selectors/get_deselected_data_series';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { computeLegend } from '../../legend/legend';
import { computeSeriesDomainsSelector } from './compute_series_domains';
import { getSeriesColorsSelector } from './get_series_color_map';
Expand All @@ -37,14 +38,24 @@ export const computeLegendSelector = createCachedSelector(
getSeriesColorsSelector,
getAxisSpecsSelector,
getDeselectedSeriesSelector,
getSettingsSpecSelector,
],
(seriesSpecs, seriesDomainsAndData, chartTheme, seriesColors, axesSpecs, deselectedDataSeries): LegendItem[] =>
(
seriesSpecs,
seriesDomainsAndData,
chartTheme,
seriesColors,
axesSpecs,
deselectedDataSeries,
{ showLegendExtra },
): LegendItem[] =>
computeLegend(
seriesDomainsAndData.seriesCollection,
seriesColors,
seriesSpecs,
chartTheme.colors.defaultVizColor,
axesSpecs,
showLegendExtra,
deselectedDataSeries,
),
)(getChartIdSelector);
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ export const getLegendItemsLabelsSelector = createCachedSelector(
[computeLegendSelector, getSettingsSpecSelector],
(legendItems, { showLegendExtra }): LegendItemLabel[] =>
legendItems.map(({ label, defaultExtra }) => {
if (defaultExtra?.formatted != null) {
return { label: `${label}${showLegendExtra ? defaultExtra.formatted : ''}`, depth: 0 };
if (defaultExtra?.legendSizingLabel != null) {
return { label: `${label}${showLegendExtra ? defaultExtra.legendSizingLabel : ''}`, depth: 0 };
}
return { label, depth: 0 };
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ describe('Type Checks', () => {
key: 'specId:{bars},colors:{a}',
specId: 'bars',
},
defaultExtra: { raw: 6, formatted: '6.00' },
defaultExtra: { raw: 6, formatted: '6.00', legendSizingLabel: '6.00' },
isSeriesHidden: true,
},
{
Expand All @@ -151,7 +151,7 @@ describe('Type Checks', () => {
key: 'specId:{bars},colors:{b}',
specId: 'bars',
},
defaultExtra: { raw: 2, formatted: '2.00' },
defaultExtra: { raw: 2, formatted: '2.00', legendSizingLabel: '2.00' },
isSeriesHidden: true,
},
];
Expand All @@ -166,7 +166,7 @@ describe('Type Checks', () => {
key: 'specId:{bars},colors:{a}',
specId: 'bars',
},
defaultExtra: { raw: 6, formatted: '6.00' },
defaultExtra: { raw: 6, formatted: '6.00', legendSizingLabel: '6.00' },
isSeriesHidden: false,
},
{
Expand All @@ -176,7 +176,7 @@ describe('Type Checks', () => {
key: 'specId:{bars},colors:{b}',
specId: 'bars',
},
defaultExtra: { raw: 2, formatted: '2.00' },
defaultExtra: { raw: 2, formatted: '2.00', legendSizingLabel: '2.00' },
isSeriesHidden: true,
},
];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import { SeriesKey, SeriesIdentifier } from '../../../../commons/series_id';
import { Scale } from '../../../../scales';
import { ScaleType } from '../../../../scales/constants';
import { OrderBy } from '../../../../specs/settings';
import { mergePartial, Rotation, Color, isUniqueArray } from '../../../../utils/commons';
import { CurveType } from '../../../../utils/curves';
Expand Down Expand Up @@ -139,9 +138,7 @@ function getLastValues(
xDomain: XDomain,
): Map<SeriesKey, LastValues> {
const lastValues = new Map<SeriesKey, LastValues>();
if (xDomain.scaleType === ScaleType.Ordinal) {
return lastValues;
}

// we need to get the latest
formattedDataSeries.stacked.forEach(({ dataSeries, stackMode }) => {
dataSeries.forEach((series) => {
Expand Down
1 change: 1 addition & 0 deletions packages/osd-charts/src/commons/legend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export type LegendItem = {
defaultExtra?: {
raw: number | null;
formatted: number | string | null;
legendSizingLabel: number | string | null;
};
// TODO: Remove when partition layers are toggleable
isToggleable?: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@ export const Example = () => {
return (
<Chart className="story-chart">
<Settings
theme={{
legend: {
// Used to allow space for long formatting
spacingBuffer: 60,
},
}}
showLegendExtra
showLegend={showLegend}
tooltip={{
Expand Down
4 changes: 3 additions & 1 deletion packages/osd-charts/stories/mixed/6_fitting_stacked.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/

import { select, number } from '@storybook/addon-knobs';
import numeral from 'numeral';
import React from 'react';

import { AreaSeries, Axis, Chart, CurveType, Position, ScaleType, Settings, Fit, StackMode } from '../../src';
Expand Down Expand Up @@ -158,6 +159,7 @@ export const Example = () => {
const value = number('Explicit value (using Fit.Explicit)', 5);
const xScaleType = dataKey === 'ordinal' ? ScaleType.Ordinal : ScaleType.Linear;
const rng = getRandomNumberGenerator();
const tickFormat = stackMode === 'percentage' ? (d: any) => numeral(d).format('0[.]00%') : undefined;
return (
<Chart className="story-chart">
<Settings
Expand All @@ -172,7 +174,7 @@ export const Example = () => {
}}
/>
<Axis id="bottom" position={Position.Bottom} title="Bottom axis" showOverlappingTicks />
<Axis id="left" title="Left axis" position={Position.Left} />
<Axis id="left" title="Left axis" position={Position.Left} tickFormat={tickFormat} />
<AreaSeries
id="test3"
xScaleType={xScaleType}
Expand Down

0 comments on commit 74bcbad

Please sign in to comment.