feat(legend): display pie chart legend extra

[shahzad31]

Summary

Fixes: #638

Pie chart will display values in legends when legend extra prop is passed

Checklist

• Proper documentation or storybook story was added for features that require explanation or tutorials

[codecov-io]

Codecov Report

Merging #939 (dac6d17) into master (dce139a) will increase coverage by 0.07%.
The diff coverage is 91.89%.

@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
+ Coverage   70.80%   70.87%   +0.07%     
==========================================
  Files         345      346       +1     
  Lines       10998    11028      +30     
  Branches     2314     2418     +104     
==========================================
+ Hits         7787     7816      +29     
- Misses       3198     3199       +1     
  Partials       13       13              

Flag	Coverage Δ
unittests	70.87% <91.89%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/components/legend/utils.ts	33.33% <33.33%> (-3.04%)	⬇️
.../chart_types/partition_chart/state/chart_state.tsx	71.66% <66.66%> (ø)
...on_chart/state/selectors/get_legend_items_extra.ts	100.00% <100.00%> (ø)
src/components/legend/legend_item.tsx	92.30% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dce139a...dac6d17. Read the comment docs.

[nickofthyme]

This is awesome, thanks for your help! 🎉

Everything looks good, I found 2 issues thus far.

First, it looks like the indent is not correctly accounting for the width of the legend item and the value is being pushed to the right. You can fix this by changing marginLeft here to paddingLeft.

elastic-charts/src/components/legend/legend_item.tsx

Lines 217 to 221 in 3c823fd

	const style = item.depth
	? {
	marginLeft: LEGEND_HIERARCHY_MARGIN * (item.depth ?? 0),
	}
	: undefined;

Secondly, the values of the nested items appear to be incorrect. Notice below that Asia is $677 Bn in the chart but $174 Bn in the legend. It looks like using the flattened values will override keys in separate branches, you likely need to do this logic on the tree form because in the flattened version you lose the reference to the parent key.

[markov00]

Secondly, the values of the nested items appear to be incorrect. Notice below that Asia is $677 Bn in the chart but $174 Bn in the legend. It looks like using the flattened values will override keys in separate branches, you likely need to do this logic on the tree form because in the flattened version you lose the reference to the parent key.

Hey @monfera can you please check this PR and see if your current work on fixing the hierarchical legend fixes also this value? Thanks

[monfera]

@markov00 we touched on this on our call with @nickofthyme and he suggests the values are OK as they are and no action is needed in relation to this item. Nick, please tell if I misinterpreted it

[nickofthyme]

@monfera that's correct. @shahzad I can take a look today if you'd like.

[shahzad31]

@nickofthyme sure , please go ahead, i am unable to figure out, how to parse legends info in a tree form.

[nickofthyme]

@monfera I lied 🤥. After looking into this, the way we display the legend I don't know the parent "key" so I would need your changes from #947

it's me

[monfera]

Great PR 🎉 Only requesting some changes because of the attempt at hoisting some definition, namely the concept of Category, see category.ts in the legend strategy PR. It wasn't a fully arbitrary choice; series keys and legend labels have commonality. Basically, both represent category. So it fell out, besides my slant toward moving to more semantic concepts than string. Looking forward to nominal typing, or if it doesn't come soon enough, branded types in elastic-charts, because string is not very safe, now a lot of things that are semantically related just happen to be all strings

[monfera]

Suggested change

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

[monfera]

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

[nickofthyme]

69c7dc8

[monfera]

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);

[nickofthyme]

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.

[nickofthyme]

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

[monfera]

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

[monfera]

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

[nickofthyme]

See dac6d17

[monfera]

Looks good and super useful, great job 🎉 and thanks for the changes too! Good to merge on green

[markov00]

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀