fix(partition): getLegendItemsExtra no longer assumes a singleton #1199

monfera · 2021-06-08T22:18:18Z

Summary

The legend extras (values) now show up in the legend for slices of all panels of a partition small multiple. In the past, the collection of values assumed the presence of one partition only.

BREAKING CHANGE

The LegendPath array in event callbacks gained a new element for partition charts, which represents the positional index and stringified value of the groupBy categorical breakdown, if any; NULL_SMALL_MULTIPLES_KEY if not applicable, ie. it's not part of a small multiples esemble.

Details

Go to the http://localhost:9001/?path=/story/small-multiples-alpha--sunbursts knobs to activate Show legend extra. All values should show up

Issues

The issue was identified via searching the code for todo comments left in the partition small multiples, referencing singletons.

Checklist

The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
Whenever possible, please check if the closing issue is connected to a running GH project
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

…-fix

nickofthyme

Changes LGTM, tested locally. How do the legend values represent values across partitions? Are they all just lumps together under the same legend path?

packages/charts/src/chart_types/partition_chart/partition.test.tsx

markov00

I tested it locally and compared it to the current version and we can finally see all the legend extra values on small multiples too.

The is a major thing here to note: this introduces a BREAKING CHANGE: the LegendPath is actually exposed as API, the API is not changed itself, but the actual value now includes one more depth level (the actual small multiple one).

Are we sure we want to add an unclear {index: 0, value: ""} at level 0 when we are not using small multiples?
In any case this still a BREAKING CHANGE

markov00 · 2021-06-10T12:48:01Z

packages/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.test.ts

@@ -97,7 +97,7 @@ describe('Partition - Legend item extra values', () => {
    MockStore.addSpecs([settings, spec], store);

    const extraValues = getLegendItemsExtra(store.getState());
-    expect([...extraValues.keys()]).toEqual(['0', '0__0', '0__1']);
+    expect([...extraValues.keys()]).toEqual(['0__0', '0__0__0', '0__0__1']);


["😐","😐😐","😐"]

the API is not changed itself, but the actual value now includes one more depth level

I agree, I haven't considered it as the type signature hasn't changed.

Are we sure we want to add an unclear {index: 0, value: ""} at level 0 when we are not using small multiples?

Looks possible to avoid this. I very briefly thought about it while changing the code and brushed it aside as it also creates a special case that the user code needs to handle with conditional branching in their event handler depending on whether or not it's a small multiples ensemble. When there's no clear benefit either way, I'm leaning toward avoiding corner cases (of eg. singleton chart) as it complicates code paths, and sometimes forces types into union types in our code (not in this case) or in the userland, which, all things being equal, is good to avoid.

Nonetheless I'm open to conditionally adding the first layer if you have an argument for it

I agree with you about not having corner cases.
I'm wondering if useful to have a more descriptive value there, a value: null o value:__no_sm__ can help understand better what that path value is actually about? if not we can probably document the LegendPath type describing that the first 2 elements are always [sm category, an empty root node, .....]

…LTIPLES_KEY`

…-fix # Conflicts: # packages/charts/src/chart_types/partition_chart/state/selectors/picked_shapes.test.ts

rshen91

LGTM tested locally!

monfera · 2021-06-10T22:12:58Z

jenkins test this please

# [31.0.0](v30.2.0...v31.0.0) (2021-06-29) ### Bug Fixes * **xy:** render gridlines behind axis ([#1204](#1204)) ([38ebe2d](38ebe2d)), closes [#1203](#1203) * memory leak related to re-reselect cache ([#1201](#1201)) ([02025cf](02025cf)) * **partition:** getLegendItemsExtra no longer assumes a singleton ([#1199](#1199)) ([100145b](100145b)) ### Features * **annotations:** option to render rect annotations outside chart ([#1207](#1207)) ([4eda382](4eda382)) * **heatmap:** enable brushing on categorical charts ([#1212](#1212)) ([10c3493](10c3493)), closes [#1170](#1170) [#1171](#1171) * **xy:** add onPointerUpdate debounce and trigger options ([#1194](#1194)) ([a9a9b25](a9a9b25)) ### BREAKING CHANGES * **xy:** the `PointerOverEvent` type now extends `ProjectedValues` and drops value. This effectively replaces value with `x`, `y`, `smVerticalValue` and `smHorizontalValue`.

nickofthyme · 2021-06-29T04:28:28Z

🎉 This PR is included in version 31.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [31.0.0](elastic/elastic-charts@v30.2.0...v31.0.0) (2021-06-29) ### Bug Fixes * **xy:** render gridlines behind axis ([opensearch-project#1204](elastic/elastic-charts#1204)) ([bf9ccbd](elastic/elastic-charts@bf9ccbd)), closes [#1203](elastic/elastic-charts#1203) * memory leak related to re-reselect cache ([opensearch-project#1201](elastic/elastic-charts#1201)) ([8cb6876](elastic/elastic-charts@8cb6876)) * **partition:** getLegendItemsExtra no longer assumes a singleton ([opensearch-project#1199](elastic/elastic-charts#1199)) ([ecbcc1e](elastic/elastic-charts@ecbcc1e)) ### Features * **annotations:** option to render rect annotations outside chart ([opensearch-project#1207](elastic/elastic-charts#1207)) ([ddffc00](elastic/elastic-charts@ddffc00)) * **heatmap:** enable brushing on categorical charts ([opensearch-project#1212](elastic/elastic-charts#1212)) ([5c426b3](elastic/elastic-charts@5c426b3)), closes [opensearch-project#1170](elastic/elastic-charts#1170) [opensearch-project#1171](elastic/elastic-charts#1171) * **xy:** add onPointerUpdate debounce and trigger options ([opensearch-project#1194](elastic/elastic-charts#1194)) ([aa068f6](elastic/elastic-charts@aa068f6)) ### BREAKING CHANGES * **xy:** the `PointerOverEvent` type now extends `ProjectedValues` and drops value. This effectively replaces value with `x`, `y`, `smVerticalValue` and `smHorizontalValue`.

fix: getLegendItemsExtra no longer assumes a singleton

bfbcd08

monfera added bug Something isn't working :partition Partition/PieChart/Donut/Sunburst/Treemap chart related :small multiples Small multiples/trellising related issues labels Jun 8, 2021

monfera added 2 commits June 9, 2021 19:23

fix: icicle and flame should ignore the grouping

46f13d6

Merge remote-tracking branch 'origin/master' into singleton-partition…

ef40b2d

…-fix

monfera requested review from markov00, nickofthyme and rshen91 June 10, 2021 05:28

monfera added the :legend Legend related issue label Jun 10, 2021

chore: remove disused function

874b94a

nickofthyme approved these changes Jun 10, 2021

View reviewed changes

packages/charts/src/chart_types/partition_chart/partition.test.tsx Outdated Show resolved Hide resolved

markov00 reviewed Jun 10, 2021

View reviewed changes

markov00 self-requested a review June 10, 2021 13:37

monfera added 3 commits June 10, 2021 20:40

refactor: exported HIERARCHY_ROOT_KEY and also added `NULL_SMALL_MU…

da812fa

…LTIPLES_KEY`

chore: api update and api generator path update

afc546b

Merge remote-tracking branch 'origin/master' into singleton-partition…

d7a18fd

…-fix # Conflicts: # packages/charts/src/chart_types/partition_chart/state/selectors/picked_shapes.test.ts

rshen91 approved these changes Jun 10, 2021

View reviewed changes

monfera merged commit 100145b into elastic:master Jun 10, 2021

nickofthyme added the released Issue released publicly label Jun 29, 2021

markov00 mentioned this pull request Jun 29, 2021

Update dependency @elastic/charts to v31.0.0 (master) elastic/kibana#102078

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(partition): getLegendItemsExtra no longer assumes a singleton #1199

fix(partition): getLegendItemsExtra no longer assumes a singleton #1199

monfera commented Jun 8, 2021 •

edited

Loading

nickofthyme left a comment

markov00 left a comment

markov00 Jun 10, 2021

monfera Jun 10, 2021 •

edited

Loading

markov00 Jun 10, 2021

rshen91 left a comment

monfera commented Jun 10, 2021

nickofthyme commented Jun 29, 2021

fix(partition): getLegendItemsExtra no longer assumes a singleton #1199

fix(partition): getLegendItemsExtra no longer assumes a singleton #1199

Conversation

monfera commented Jun 8, 2021 • edited Loading

Summary

BREAKING CHANGE

Details

Issues

Checklist

nickofthyme left a comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 Jun 10, 2021

Choose a reason for hiding this comment

monfera Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

markov00 Jun 10, 2021

Choose a reason for hiding this comment

rshen91 left a comment

Choose a reason for hiding this comment

monfera commented Jun 10, 2021

nickofthyme commented Jun 29, 2021

monfera commented Jun 8, 2021 •

edited

Loading

monfera Jun 10, 2021 •

edited

Loading