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): legend sizes with ordinal data #867

Merged
merged 5 commits into from
Oct 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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.
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 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 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 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,
},
Comment on lines -113 to -116
Copy link
Collaborator

Choose a reason for hiding this comment

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

Careful with extracting this, the value here is y0 not y1 like the function returns

Copy link
Contributor Author

@rshen91 rshen91 Oct 19, 2020

Choose a reason for hiding this comment

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

Thanks! Commit a054fdc for changes

defaultExtra: getLegendExtra(showLegendExtra, spec.xScaleType, formatter, 'y0', lastValue),
});
}
});
Expand Down
1 change: 1 addition & 0 deletions src/chart_types/xy_chart/rendering/rendering.test.ts
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
8 changes: 0 additions & 8 deletions src/chart_types/xy_chart/state/chart_state.test.ts
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
13 changes: 12 additions & 1 deletion src/chart_types/xy_chart/state/selectors/compute_legend.ts
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
8 changes: 4 additions & 4 deletions src/chart_types/xy_chart/state/utils/common.test.ts
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
5 changes: 1 addition & 4 deletions src/chart_types/xy_chart/state/utils/utils.ts
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 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
6 changes: 0 additions & 6 deletions stories/axes/6a_different_tooltip_formatter.tsx
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 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