Skip to content

Commit

Permalink
fix: group legend items by label and color (#999)
Browse files Browse the repository at this point in the history
This commit fixes the duplicated legend elements when displaying small multiples.
The legend item concept should allow the user to uniquely link a geometry color to a label. There are situations where multiple geometries or series are colored with the same color, like in a small multiple, and all of these series are related to the same metric for example. This PR removes any duplicate of the key color, label in the legend item, and introduce the ability to have multiple series referenced by the same legend item.

BREAKING CHANGE: The `LegendActionProps` and the `LegendColorPickerProps`, used to add actions and color picker through the legend now receive an array of `SeriesIdentifiers`
  • Loading branch information
markov00 committed Feb 16, 2021
1 parent a1abb85 commit 5d32f23
Show file tree
Hide file tree
Showing 61 changed files with 607 additions and 1,425 deletions.
2 changes: 1 addition & 1 deletion .playground/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
<style>
html,
body {
background: blanchedalmond !important;
background: white !important;
width: 100%;
height: 100%;
overflow: hidden;
Expand Down
1 change: 1 addition & 0 deletions .playground/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import React from 'react';
import ReactDOM from 'react-dom';

import '../src/theme_light.scss';
import '../node_modules/@elastic/eui/dist/eui_theme_light.css';
import { Playground } from './playground';

ReactDOM.render(<Playground />, document.getElementById('root') as HTMLElement);
10 changes: 10 additions & 0 deletions .playground/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ module.exports = {
transpileOnly: true,
},
},
{
test: /\.css$/,
use: [
'style-loader',
{
loader: 'css-loader',
options: { importLoaders: 1 },
},
],
},
{
test: /\.scss$/,
use: [
Expand Down
17 changes: 13 additions & 4 deletions api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1207,7 +1207,7 @@ export type LegendAction = ComponentType<LegendActionProps>;
export interface LegendActionProps {
color: string;
label: string;
series: SeriesIdentifier;
series: SeriesIdentifier[];
}

// Warning: (ae-missing-release-tag) "LegendColorPicker" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
Expand All @@ -1223,13 +1223,13 @@ export interface LegendColorPickerProps {
color: Color;
onChange: (color: Color | null) => void;
onClose: () => void;
seriesIdentifier: SeriesIdentifier;
seriesIdentifiers: SeriesIdentifier[];
}

// Warning: (ae-missing-release-tag) "LegendItemListener" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export type LegendItemListener = (series: SeriesIdentifier | null) => void;
export type LegendItemListener = (series: SeriesIdentifier[]) => void;

// @public (undocumented)
export type LegendPath = LegendPathElement[];
Expand Down Expand Up @@ -1778,6 +1778,11 @@ export type SeriesIdentifier = {
key: SeriesKey;
};

// Warning: (ae-missing-release-tag) "SeriesKey" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public
export type SeriesKey = CategoryKey;

// @public (undocumented)
export type SeriesName = string | number | null;

Expand Down Expand Up @@ -2163,6 +2168,11 @@ export type TickStyle = StrokeStyle & Visible & {
// @public (undocumented)
export function timeFormatter(format: string): TickFormatter;

// Warning: (ae-missing-release-tag) "toEntries" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public
export function toEntries<T extends Record<string, string>, S>(array: T[], accessor: keyof T, staticValue: S): Record<string, S>;

// @public
export interface TooltipInfo {
header: TooltipValue | null;
Expand Down Expand Up @@ -2348,7 +2358,6 @@ export type YDomainRange = YDomainBase & DomainRange;
// src/chart_types/heatmap/layout/types/config_types.ts:62:5 - (ae-forgotten-export) The symbol "TextBaseline" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/layout/types/config_types.ts:130:5 - (ae-forgotten-export) The symbol "TimeMs" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/layout/types/config_types.ts:131:5 - (ae-forgotten-export) The symbol "AnimKeyframe" needs to be exported by the entry point index.d.ts
// src/common/series_id.ts:40:3 - (ae-forgotten-export) The symbol "SeriesKey" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
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.
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.
3 changes: 2 additions & 1 deletion src/chart_types/heatmap/state/selectors/compute_legend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ export const computeLegendSelector = createCachedSelector(
return {
color,
label: `> ${tick}`,
seriesIdentifier,
seriesIdentifiers: [seriesIdentifier],
isSeriesHidden: deselectedDataSeries.some((dataSeries) => dataSeries.key === seriesIdentifier.key),
isToggleable: true,
path: [{ index: 0, value: seriesIdentifier.key }],
keys: [],
};
});
},
Expand Down
3 changes: 2 additions & 1 deletion src/chart_types/partition_chart/layout/utils/legend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ export function getLegendItems(
childId: dataName,
depth: useHierarchicalLegend ? depth - 1 : 0,
path,
seriesIdentifier: { key: dataName, specId: id },
seriesIdentifiers: [{ key: dataName, specId: id }],
keys: [],
};
});
}
39 changes: 26 additions & 13 deletions src/chart_types/partition_chart/partition.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 0,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
seriesIdentifiers: [{ key: 'A', specId: 'spec1' }],
keys: [],
},
{
childId: 'A',
Expand All @@ -115,7 +116,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 1,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
seriesIdentifiers: [{ key: 'A', specId: 'spec1' }],
keys: [],
},
{
childId: 'B',
Expand All @@ -127,7 +129,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 1,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
seriesIdentifiers: [{ key: 'B', specId: 'spec1' }],
keys: [],
},
{
childId: 'B',
Expand All @@ -138,7 +141,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 0,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
seriesIdentifiers: [{ key: 'B', specId: 'spec1' }],
keys: [],
},
{
childId: 'A',
Expand All @@ -150,7 +154,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 1,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
seriesIdentifiers: [{ key: 'A', specId: 'spec1' }],
keys: [],
},
{
childId: 'B',
Expand All @@ -162,7 +167,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 1,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
seriesIdentifiers: [{ key: 'B', specId: 'spec1' }],
keys: [],
},
{
childId: 'C',
Expand All @@ -173,7 +179,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 0,
label: 'C',
seriesIdentifier: { key: 'C', specId: 'spec1' },
seriesIdentifiers: [{ key: 'C', specId: 'spec1' }],
keys: [],
},
{
childId: 'A',
Expand All @@ -185,7 +192,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 1,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
seriesIdentifiers: [{ key: 'A', specId: 'spec1' }],
keys: [],
},
{
childId: 'B',
Expand All @@ -197,7 +205,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 1,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
seriesIdentifiers: [{ key: 'B', specId: 'spec1' }],
keys: [],
},
]);
});
Expand All @@ -220,7 +229,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 0,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
seriesIdentifiers: [{ key: 'A', specId: 'spec1' }],
keys: [],
},
{
childId: 'A',
Expand All @@ -242,7 +252,8 @@ describe('Retain hierarchy even with arbitrary names', () => {

depth: 1,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
seriesIdentifiers: [{ key: 'A', specId: 'spec1' }],
keys: [],
},
]);
});
Expand All @@ -265,7 +276,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 0,
label: 'C',
seriesIdentifier: { key: 'C', specId: 'spec1' },
seriesIdentifiers: [{ key: 'C', specId: 'spec1' }],
keys: [],
},
{
childId: 'B',
Expand All @@ -286,7 +298,8 @@ describe('Retain hierarchy even with arbitrary names', () => {
],
depth: 1,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
seriesIdentifiers: [{ key: 'B', specId: 'spec1' }],
keys: [],
},
]);
});
Expand Down
2 changes: 1 addition & 1 deletion src/chart_types/xy_chart/legend/legend.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ describe('Legends', () => {
formattedDataSeries: [{ key, specId }],
} = computeSeriesDomainsSelector(store.getState());

store.dispatch(onToggleDeselectSeriesAction({ key, specId }));
store.dispatch(onToggleDeselectSeriesAction([{ key, specId }]));
const legend = computeLegendSelector(store.getState());
const visibility = legend.map((item) => !item.isSeriesHidden);
expect(visibility).toEqual([false, true]);
Expand Down
35 changes: 30 additions & 5 deletions src/chart_types/xy_chart/legend/legend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ 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';
import { groupBy } from '../utils/group_data_series';
import {
getSeriesIndex,
getSeriesName,
Expand Down Expand Up @@ -104,11 +105,21 @@ export function computeLegend(
const legendItems: LegendItem[] = [];

dataSeries.forEach((series) => {
const { specId } = series;
const { specId, yAccessor } = series;
const banded = isDataSeriesBanded(series);
const key = getSeriesKey(series, series.groupId);
const spec = getSpecsById<BasicSeriesSpec>(specs, specId);
const color = seriesColors.get(key) || defaultColor;
const dataSeriesKey = getSeriesKey(
{
specId: series.specId,
yAccessor: series.yAccessor,
splitAccessors: series.splitAccessors,
},
series.groupId,
);

const color = seriesColors.get(dataSeriesKey) || defaultColor;

const hasSingleSeries = dataSeries.length === 1;
const name = getSeriesName(series, hasSingleSeries, false, spec);
const isSeriesHidden = deselectedDataSeries ? getSeriesIndex(deselectedDataSeries, series) >= 0 : false;
Expand All @@ -129,26 +140,28 @@ export function computeLegend(
legendItems.push({
color,
label: labelY1,
seriesIdentifier,
seriesIdentifiers: [seriesIdentifier],
childId: BandedAccessorType.Y1,
isSeriesHidden,
isItemHidden: hideInLegend,
isToggleable: true,
defaultExtra: getLegendExtra(showLegendExtra, spec.xScaleType, formatter, 'y1', lastValue),
path: [{ index: 0, value: seriesIdentifier.key }],
keys: [specId, spec.groupId, yAccessor, ...series.splitAccessors.values()],
});
if (banded) {
const labelY0 = getBandedLegendItemLabel(name, BandedAccessorType.Y0, postFixes);
legendItems.push({
color,
label: labelY0,
seriesIdentifier,
seriesIdentifiers: [seriesIdentifier],
childId: BandedAccessorType.Y0,
isSeriesHidden,
isItemHidden: hideInLegend,
isToggleable: true,
defaultExtra: getLegendExtra(showLegendExtra, spec.xScaleType, formatter, 'y0', lastValue),
path: [{ index: 0, value: seriesIdentifier.key }],
keys: [specId, spec.groupId, yAccessor, ...series.splitAccessors.values()],
});
}
});
Expand All @@ -159,5 +172,17 @@ export function computeLegend(
return defaultXYLegendSeriesSort(aDs, bDs);
});

return legendItems.sort((a, b) => legendSortFn(a.seriesIdentifier, b.seriesIdentifier));
return groupBy(
legendItems.sort((a, b) => legendSortFn(a.seriesIdentifiers[0], b.seriesIdentifiers[0])),
({ keys, childId }) => {
return [...keys, childId].join('__'); // childId is used for band charts
},
true,
).map((d) => {
return {
...d[0],
seriesIdentifiers: d.map(({ seriesIdentifiers: [s] }) => s),
path: d.map(({ path: [p] }) => p),
};
});
}
14 changes: 9 additions & 5 deletions src/chart_types/xy_chart/rendering/rendering.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,22 +133,26 @@ describe('Rendering utils', () => {
const highlightedLegendItem: LegendItem = {
color: '',
label: '',
seriesIdentifier,
seriesIdentifiers: [seriesIdentifier],
isSeriesHidden: false,
defaultExtra: {
formatted: null,
raw: null,
legendSizingLabel: null,
},
path: [],
keys: [],
};

const unhighlightedLegendItem: LegendItem = {
...highlightedLegendItem,
seriesIdentifier: {
...seriesIdentifier,
key: 'not me',
},
seriesIdentifiers: [
{
...seriesIdentifier,
key: 'not me',
},
],
keys: [],
};

const sharedThemeStyle: SharedGeometryStateStyle = {
Expand Down
4 changes: 3 additions & 1 deletion src/chart_types/xy_chart/rendering/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ export function getGeometryStateStyle(
const { default: defaultStyles, highlighted, unhighlighted } = sharedGeometryStyle;

if (highlightedLegendItem) {
const isPartOfHighlightedSeries = seriesIdentifier.key === highlightedLegendItem.seriesIdentifier.key;
const isPartOfHighlightedSeries = highlightedLegendItem.seriesIdentifiers.some(
({ key }) => key === seriesIdentifier.key,
);

return isPartOfHighlightedSeries ? highlighted : unhighlighted;
}
Expand Down
Loading

0 comments on commit 5d32f23

Please sign in to comment.