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

feat(legend): display pie chart legend extra #939

Merged
merged 12 commits into from
Jan 20, 2021
7 changes: 3 additions & 4 deletions src/chart_types/partition_chart/state/chart_state.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { Partition } from '../renderer/canvas/partition';
import { HighlighterFromHover } from '../renderer/dom/highlighter_hover';
import { HighlighterFromLegend } from '../renderer/dom/highlighter_legend';
import { computeLegendSelector } from './selectors/compute_legend';
import { getLegendItemsExtra } from './selectors/get_legend_items_extra';
import { getLegendItemsLabels } from './selectors/get_legend_items_labels';
import { isTooltipVisibleSelector } from './selectors/is_tooltip_visible';
import { createOnElementClickCaller } from './selectors/on_element_click_caller';
Expand All @@ -37,8 +38,6 @@ import { createOnElementOverCaller } from './selectors/on_element_over_caller';
import { getPieSpec } from './selectors/pie_spec';
import { getTooltipInfoSelector } from './selectors/tooltip';

const EMPTY_MAP = new Map();

/** @internal */
export class PartitionState implements InternalChartState {
chartType = ChartTypes.Partition;
Expand Down Expand Up @@ -80,8 +79,8 @@ export class PartitionState implements InternalChartState {
return computeLegendSelector(globalState);
}

getLegendExtraValues() {
return EMPTY_MAP;
getLegendExtraValues(globalState: GlobalChartState) {
return getLegendItemsExtra(globalState);
}

chartRenderer(containerRef: BackwardRef, forwardStageRef: RefObject<HTMLCanvasElement>) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,14 @@ export const computeLegendSelector = createCachedSelector(

items.sort(compareTreePaths);

return items.map<LegendItem>(({ dataName, fillColor, depth }) => {
return items.map<LegendItem>(({ dataName, fillColor, depth, path }) => {
const formatter = pieSpec.layers[depth - 1]?.nodeLabel ?? identity;
return {
color: fillColor,
label: formatter(dataName),
dataName,
childId: dataName,
path,
depth: forceFlatLegend ? 0 : depth - 1,
seriesIdentifier: { key: dataName, specId: pieSpec.id },
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import createCachedSelector from 're-reselect';

import { LegendItemExtraValues } from '../../../../commons/legend';
import { SeriesKey } from '../../../../commons/series_id';
import { SettingsSpec } from '../../../../specs';
import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { HierarchyOfArrays, CHILDREN_KEY } from '../../layout/utils/group_by_rollup';
import { PartitionSpec } from '../../specs';
import { getPieSpec } from './pie_spec';
import { getTree } from './tree';

/** @internal */
export const getLegendItemsExtra = createCachedSelector(
[getPieSpec, getSettingsSpecSelector, getTree],
(pieSpec, { legendMaxDepth }, tree): Map<SeriesKey, LegendItemExtraValues> => {
const legendExtraValues = new Map<SeriesKey, LegendItemExtraValues>();

if (!pieSpec) {
return legendExtraValues;
}
if (isInvalidLegendMaxDepth(legendMaxDepth)) {
return legendExtraValues;
}

return getExtraValueMap(pieSpec, tree);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!pieSpec) {
return legendExtraValues;
}
if (isInvalidLegendMaxDepth(legendMaxDepth)) {
return legendExtraValues;
}
return getExtraValueMap(pieSpec, tree);
return pieSpec && !isInvalidLegendMaxDepth(legendMaxDepth))
? getExtraValueMap(pieSpec, tree)
: legendExtraValues;

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'd prefer positive predicates in general, eg. isValidLegendMaxDepth over !isInvalidLegendMaxDepth

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

@monfera monfera Jan 19, 2021

Choose a reason for hiding this comment

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

Good changes here, would still prefer a positive predicate over a negative one, though totally not a blocker.

I've a question here: this extracts, and only keys on the concatenated value ie. series keys; won't this approach need the structure information downstream of this? The legend strategies PR discontinued the 1:1 relation between data label (textual name) and identifying node(s);veven for just highlighting the value, at least for non-single layer charts such as partition charts. I'll look at it in more detail tomorrow, just wanted to check if/how this info won't be needed, ie. why eg. not key on the concatentated {index, value} tuples rather than on just the value.

keys.set(path.map(({ value: v }) => v).join('__'), values);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just assume not mess with existing naming (i.e. isInvalidLegendMaxDepth) because there's likely a lot of this around. I typically use positive or negative such that I don't have to ! it.

Copy link
Collaborator

@nickofthyme nickofthyme Jan 20, 2021

Choose a reason for hiding this comment

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

Yes you are right about the value that should be index instead. See 13811eb

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Fair enough not to add possibly sweeping changes to this PR. We could maybe brainstorm about whether we should prefer positive predicates at all, not just the general avoidance of non or un but also some legit words like invalid; there's no specific line we can draw between negative and positive words and it might be something noone else cares about, it's a miniscule stylistic thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Re the value vs index change, the current test suite is insensitive to whether it yields the desired thing, it'd be neat to make it fail if the info is insufficient, I guess it depends on mock structure to trigger a difference

Copy link
Collaborator

Choose a reason for hiding this comment

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

See dac6d17

},
)(getChartIdSelector);

/**
* Check if the legendMaxDepth from settings is not a valid number (NaN or <=0)
*
* @param legendMaxDepth - SettingsSpec['legendMaxDepth']
*/
function isInvalidLegendMaxDepth(legendMaxDepth: SettingsSpec['legendMaxDepth']): boolean {
return typeof legendMaxDepth === 'number' && (Number.isNaN(legendMaxDepth) || legendMaxDepth <= 0);
}

/**
* Creates flat extra value map from nested key path
*/
function getExtraValueMap(
{ layers, valueFormatter }: Pick<PartitionSpec, 'layers' | 'valueFormatter'>,
tree: HierarchyOfArrays,
keys: Map<SeriesKey, LegendItemExtraValues> = new Map(),
): Map<SeriesKey, LegendItemExtraValues> {
if (tree.length === 0) {
return keys;
}

monfera marked this conversation as resolved.
Show resolved Hide resolved
for (let i = 0; i < tree.length; i++) {
const branch = tree[i];
const [key, arrayNode] = branch;
const { value, path, [CHILDREN_KEY]: children } = arrayNode;

if (key != null) {
const values: LegendItemExtraValues = new Map();
const formattedValue = valueFormatter ? valueFormatter(value) : value;

values.set(key, formattedValue);
keys.set(path.join('__'), values);
}

getExtraValueMap({ layers, valueFormatter }, children, keys);
}
return keys;
}
7 changes: 6 additions & 1 deletion src/commons/legend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,19 @@
import { PrimitiveValue } from '../chart_types/partition_chart/layout/utils/group_by_rollup';
import { Color } from '../utils/commons';
import { SeriesIdentifier } from './series_id';

/** @internal */
export type LegendItemChildId = string;
export type LegendItemChildId = string | number;
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved

/** @internal */
export type LegendItem = {
seriesIdentifier: SeriesIdentifier;
childId?: LegendItemChildId;
depth?: number;
/**
* Path to iterm in hierarchical legend
*/
path?: number[];
monfera marked this conversation as resolved.
Show resolved Hide resolved
color: Color;
label: string;
isSeriesHidden?: boolean;
Expand Down
4 changes: 2 additions & 2 deletions src/components/legend/legend_item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ export class LegendListItem extends Component<LegendItemProps, LegendItemState>
'echLegendItem__extra--hidden': isItemHidden,
});
const hasColorPicker = Boolean(colorPicker);
const extra = getExtra(extraValues, item, totalItems);
const extra = showExtra && getExtra(extraValues, item, totalItems);
const style = item.depth
? {
marginLeft: LEGEND_HIERARCHY_MARGIN * (item.depth ?? 0),
Expand Down Expand Up @@ -244,7 +244,7 @@ export class LegendListItem extends Component<LegendItemProps, LegendItemState>
onClick={this.handleLabelClick(seriesIdentifier)}
isSeriesHidden={isSeriesHidden}
/>
{showExtra && extra && renderExtra(extra, isSeriesHidden)}
{extra && renderExtra(extra, isSeriesHidden)}
{Action && (
<div className="echLegendItem__action">
<Action series={seriesIdentifier} color={color} label={label} />
Expand Down
4 changes: 3 additions & 1 deletion src/components/legend/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ export function getExtra(extraValues: Map<string, LegendItemExtraValues>, item:
seriesIdentifier: { key },
defaultExtra,
childId,
path,
} = item;
if (extraValues.size === 0) {
return defaultExtra?.formatted ?? '';
}
const itemExtraValues = extraValues.get(key);
const extraValueKey = path ? path.join('__') : key;
const itemExtraValues = extraValues.get(extraValueKey);
const actionExtra = (childId && itemExtraValues?.get(childId)) ?? null;
if (extraValues.size !== totalItems) {
if (actionExtra != null) {
Expand Down
9 changes: 8 additions & 1 deletion stories/legend/10_sunburst.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {

export const Example = () => {
const flatLegend = boolean('flatLegend', true);
const showLegendExtra = boolean('showLegendExtra', false);
const legendMaxDepth = number('legendMaxDepth', 2, {
min: 0,
max: 3,
Expand All @@ -43,7 +44,13 @@ export const Example = () => {

return (
<Chart className="story-chart">
<Settings showLegend flatLegend={flatLegend} legendMaxDepth={legendMaxDepth} theme={STORYBOOK_LIGHT_THEME} />
<Settings
showLegend
showLegendExtra={showLegendExtra}
flatLegend={flatLegend}
legendMaxDepth={legendMaxDepth}
theme={STORYBOOK_LIGHT_THEME}
/>
<Partition
id="spec_1"
data={mocks.miniSunburst}
Expand Down