fix(legend): hierarchical legend order should follow the tree paths by monfera · Pull Request #947 · elastic/elastic-charts

monfera · 2020-12-11T01:12:37Z

Summary

Refactoring of only local effect of some label-related lines
Closes Partition nested legends are not depicted well when there are same values per metric #918
Deactivates the anti-iterator rule

Hierarchical legends may currently occur with sunburst and treemap charts, so labeled as partition too.

Actual fix is in the last commit, the preceding ones are just a pass of incremental updates to naturally bump into what needed changing.

The same mock as in the above issue:

Checklist

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios

…e narrowed to number

codecov-io · 2020-12-11T01:16:55Z

Codecov Report

Merging #947 (f77e825) into master (03bd2f7) will increase coverage by 0.54%.
The diff coverage is 88.63%.

@@            Coverage Diff             @@
##           master     #947      +/-   ##
==========================================
+ Coverage   70.13%   70.68%   +0.54%     
==========================================
  Files         343      359      +16     
  Lines       11036    10646     -390     
  Branches     2394     2055     -339     
==========================================
- Hits         7740     7525     -215     
+ Misses       3282     3036     -246     
- Partials       14       85      +71

Flag	Coverage Δ
unittests	`?`

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

Impacted Files	Coverage Δ
...es/partition_chart/layout/types/viewmodel_types.ts	`83.33% <ø> (ø)`
...ypes/partition_chart/layout/viewmodel/viewmodel.ts	`89.06% <ø> (-1.08%)`	⬇️
.../chart_types/partition_chart/state/chart_state.tsx	`71.18% <ø> (-0.49%)`	⬇️
...on_chart/state/selectors/get_highlighted_shapes.ts	`62.50% <ø> (+6.94%)`	⬆️
src/specs/constants.ts	`100.00% <ø> (ø)`
src/state/selectors/get_legend_items_labels.ts	`50.00% <50.00%> (-25.00%)`	⬇️
src/specs/settings.tsx	`86.66% <66.66%> (-0.44%)`	⬇️
.../partition_chart/state/selectors/compute_legend.ts	`77.77% <85.00%> (-7.01%)`	⬇️
...es/partition_chart/layout/utils/group_by_rollup.ts	`86.81% <100.00%> (+0.93%)`	⬆️
src/chart_types/partition_chart/state/iterables.ts	`100.00% <100.00%> (ø)`
... and 210 more

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 03bd2f7...f77e825. Read the comment docs.

monfera · 2020-12-11T01:18:31Z

src/chart_types/partition_chart/state/selectors/compute_legend.ts

-    }, {});
-
-    const { flatLegend, legendMaxDepth, legendPosition } = settings;
+    const uniqueNames = new Set(map(({ dataName, fillColor }) => makeKey(dataName, fillColor), quadViewModel));


Here, it could've been

const uniqueNames = new Set(quadViewModel.map(({ dataName, fillColor }) => makeKey(dataName, fillColor)));

the only difference is that this version would have materialized an array. Maybe we can start adding a couple of utils for non-materializing filter, reduce etc.

monfera · 2020-12-11T01:26:52Z

src/chart_types/partition_chart/state/selectors/compute_legend.ts

-        const key = [dataName, fillColor].join('---');
-        if (uniqueNames[key] > 1 && excluded.has(key)) {
+        const key = makeKey(dataName, fillColor);
+        if (uniqueNames.has(key) && excluded.has(key)) {


Eventually we should avoid this filter loop, relying on a new operator that intersects two collections eg. two Sets.
A few ones can be added, eg. intersect, union, subtract

…dented version

markov00

Looks good to me, I've added few comments.
I've noticed that the new introduced visual regression tests baseline are not committed, you should commit them and everything else should be fine

markov00 · 2020-12-16T13:29:06Z

src/specs/settings.tsx

 /** @internal */
 export function isTooltipType(config: TooltipType | TooltipProps): config is TooltipType {
-  return typeof config === 'string';
+  return typeof config !== 'object'; // TooltipType is 'vertical'|'cross'|'follow'|'none' while TooltipProps is object


markov00 · 2020-12-16T13:34:15Z

src/chart_types/partition_chart/state/selectors/get_highlighted_shapes.ts

 const getHighlightedLegendItemKey = (state: GlobalChartState) => state.interactions.highlightedLegendItemKey;

 /** @internal */
+// why is it called highlighted... when it's a legend hover related thing, not a hover over the slices?


Are you referring to the fact that we don't describe that as a legend related thing?
Something like getLegendHighlightedSectorsSelector seems to sound better then the current one

nickofthyme

LGTM, Tested locally. This looks to be a solution to fix #939.

nickofthyme · 2020-12-15T20:02:32Z

.eslintrc.js

  rules: {
    /**
-     * depricated to be deleted
+     * deprecated to be deleted


# [24.5.0](v24.4.0...v24.5.0) (2021-01-30) ### Bug Fixes * add theme min radius to point shape ([#996](#996)) ([eb37175](eb37175)) * align tooltip z-index to EUI tooltip z-index ([#931](#931)) ([ffd626b](ffd626b)) * chart state and series functions cleanup ([#989](#989)) ([944ac6c](944ac6c)) * create unique ids for dot icons ([#971](#971)) ([e1ce768](e1ce768)) * external tooltip legend extra value sync ([#993](#993)) ([13ad05a](13ad05a)) * **legend:** disable focus and keyboard navigation for legend in partition ch… ([#952](#952)) ([03bd2f7](03bd2f7)) * **legend:** hierarchical legend order should follow the tree paths ([#947](#947)) ([f9218ad](f9218ad)), closes [#944](#944) * **legend:** remove ids for circles ([#973](#973)) ([b3f4f90](b3f4f90)) ### Features * **cursor:** improve theme styling for crosshair ([#980](#980)) ([6c4dafd](6c4dafd)) * **legend:** display pie chart legend extra ([#939](#939)) ([d14de01](d14de01)) * **legend:** add keyboard navigation ([#880](#880)) ([87c227d](87c227d)) * **partition:** Flame and icicle chart ([#965](#965)) ([3df73d0](3df73d0)) * **partition:** legend hover options ([#978](#978)) ([f810d94](f810d94)) * **xy:** support multiple point shapes on line, area and bubble charts ([#988](#988)) ([1392b7d](1392b7d))

markov00 · 2021-01-30T15:42:43Z

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

monfera added 15 commits December 11, 2020 02:02

refactor: minor improvements

36823c2

refactor: no need for the isInvalidLegendMaxDepth and the if statement

c8a629c

refactor: default legendMaxDepth is infinite depth, so its type can b…

f1f6cf2

…e narrowed to number

chore: fix TS warning of "can't be string"

fb8f575

chore: minor code elimination

951e754

refactor: flow simplification

6eb8d5b

refactor: flow simplification 2

14492dd

chore: shortened imports

68e4a27

refactor: simpler Set fill in computeLegendSelector

8605e09

refactor: avoid materialization

6078b6e

chore: switched off dubious rule centrally

d9ceb82

refactor: minor computeLegendSelector shuffles

28dbd68

perf: avoid worst case cubic sort performance

fca8f44

refactor: minor let avoidance

342a902

fix: via adding path based sorting as an alternative to flat sorting

0c913d5

monfera added wip work in progress :legend Legend related issue :partition Partition/PieChart/Donut/Sunburst/Treemap chart related labels Dec 11, 2020

monfera requested a review from markov00 December 11, 2020 01:13

monfera commented Dec 11, 2020

View reviewed changes

monfera requested review from nickofthyme and rshen91 December 11, 2020 01:22

monfera commented Dec 11, 2020

View reviewed changes

monfera added 2 commits December 14, 2020 11:04

chore: api update

d642f41

test: legend viewmodel and calculation tests

31a179e

monfera removed the wip work in progress label Dec 16, 2020

test: more image test and culled legend label ordering for the non-in…

ed739ef

…dented version

markov00 approved these changes Dec 16, 2020

View reviewed changes

test: images

d08f1b5

nickofthyme approved these changes Dec 16, 2020

View reviewed changes

.eslintrc.js

rules: {

/**

* depricated to be deleted

* deprecated to be deleted

Copy link
Copy Markdown

Collaborator

nickofthyme Dec 15, 2020

Choose a reason for hiding this comment

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

😅

nickofthyme mentioned this pull request Dec 16, 2020

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

Merged

1 task

Merge branch 'master' into sunburst-legend

f77e825

monfera merged commit f9218ad into elastic:master Dec 16, 2020

monfera mentioned this pull request Dec 16, 2020

Partition nested legends are not depicted well when there are same values per metric #918

Closed

markov00 added the released Issue released publicly label Jan 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(legend): hierarchical legend order should follow the tree paths#947

fix(legend): hierarchical legend order should follow the tree paths#947
monfera merged 20 commits intoelastic:masterfrom
monfera:sunburst-legend

monfera commented Dec 11, 2020 •

edited

Loading

Uh oh!

codecov-io commented Dec 11, 2020 •

edited

Loading

Uh oh!

monfera Dec 11, 2020

Uh oh!

monfera Dec 11, 2020

Uh oh!

markov00 left a comment

Uh oh!

markov00 Dec 16, 2020

Uh oh!

markov00 Dec 16, 2020

Uh oh!

nickofthyme left a comment

Uh oh!

nickofthyme Dec 15, 2020

Uh oh!

markov00 commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

monfera commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

codecov-io commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

monfera Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

monfera Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

markov00 Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

markov00 commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

monfera commented Dec 11, 2020 •

edited

Loading

codecov-io commented Dec 11, 2020 •

edited

Loading