fix(partition): getLegendItemsExtra no longer assumes a singleton (op…

…ensearch-project#1199) * fix: legendPath in callbacks gained an additional new element which is a BREAKING CHANGE * refactor: exported `HIERARCHY_ROOT_KEY` and also added `NULL_SMALL_MULTIPLES_KEY`
AMoo-Miki · Jun 10, 2021 · ecbcc1e · ecbcc1e
1 parent 4c9e3bf
commit ecbcc1e
Show file tree

Hide file tree

Showing 13 changed files with 120 additions and 41 deletions.
diff --git a/packages/osd-charts/.github/workflows/api_extractor_check.yaml b/packages/osd-charts/.github/workflows/api_extractor_check.yaml
@@ -20,7 +20,7 @@ jobs:
         if: ${{ failure() }}
         uses: LouisBrunner/[email protected]
         with:
-          old: api/charts.api.md
-          new: tmp/charts.api.md
+          old: packages/charts/api/charts.api.md
+          new: packages/charts/tmp/charts.api.md
           mode: deletion
           tolerance: better
diff --git a/packages/osd-charts/packages/charts/api/charts.api.md b/packages/osd-charts/packages/charts/api/charts.api.md
@@ -1027,6 +1027,9 @@ export interface HeatmapSpec extends Spec {
     ySortPredicate: Predicate;
 }
 
+// @public
+export const HIERARCHY_ROOT_KEY: Key;
+
 // @public (undocumented)
 export type HierarchyOfArrays = Array<ArrayEntry>;
 
@@ -1132,7 +1135,7 @@ export interface LegendColorPickerProps {
 // @public (undocumented)
 export type LegendItemListener = (series: SeriesIdentifier[]) => void;
 
-// @public (undocumented)
+// @public
 export type LegendPath = LegendPathElement[];
 
 // @public (undocumented)
@@ -1325,6 +1328,9 @@ export type NodeSorter = (a: ArrayEntry, b: ArrayEntry) => number;
 // @public (undocumented)
 export type NonAny = number | boolean | string | symbol | null;
 
+// @public
+export const NULL_SMALL_MULTIPLES_KEY: Key;
+
 // @public (undocumented)
 export interface Opacity {
     opacity: number;
@@ -1528,7 +1534,7 @@ export interface Postfixes {
     y1AccessorFormat?: string;
 }
 
-// @public (undocumented)
+// @public
 export type PrimitiveValue = string | number | null;
 
 // @public

diff --git a/...sd-charts/packages/charts/src/chart_types/partition_chart/layout/utils/group_by_rollup.ts b/...sd-charts/packages/charts/src/chart_types/partition_chart/layout/utils/group_by_rollup.ts
@@ -56,6 +56,7 @@ export interface NodeDescriptor {
 export type ArrayEntry = [Key, ArrayNode];
 /** @public */
 export type HierarchyOfArrays = Array<ArrayEntry>;
+
 /** @public */
 export interface ArrayNode extends NodeDescriptor {
   [CHILDREN_KEY]: HierarchyOfArrays;
@@ -65,15 +66,30 @@ export interface ArrayNode extends NodeDescriptor {
 }
 
 type HierarchyOfMaps = Map<Key, MapNode>;
+
 interface MapNode extends NodeDescriptor {
   [CHILDREN_KEY]?: HierarchyOfMaps;
   [PARENT_KEY]?: ArrayNode;
 }
 
-/** @internal */
+/**
+ * Used in the first position of a `LegendPath` array, which indicates the stringified value of the `groupBy` value
+ * in case of small multiples, but has no applicable `groupBy` for singleton (non-small-multiples) charts
+ * @public
+ */
+export const NULL_SMALL_MULTIPLES_KEY: Key = '__null_small_multiples_key__';
+
+/**
+ * Indicates that a node is the root of a specific partition chart, eg. the root of a single pie chart, or one pie
+ * chart in a small multiples setting. Used in the second position of a `LegendPath` array
+ * @public
+ */
 export const HIERARCHY_ROOT_KEY: Key = '__root_key__';
 
-/** @public */
+/**
+ * A primitive JavaScript value, possibly further restricted
+ * @public
+ */
 export type PrimitiveValue = string | number | null; // there could be more but sufficient for now
 /** @public */
 export type Key = CategoryKey;
@@ -90,26 +106,32 @@ export type NodeSorter = (a: ArrayEntry, b: ArrayEntry) => number;
 export const entryKey = ([key]: ArrayEntry) => key;
 /** @public */
 export const entryValue = ([, value]: ArrayEntry) => value;
+
 /** @public */
 export function depthAccessor(n: ArrayEntry) {
   return entryValue(n)[DEPTH_KEY];
 }
+
 /** @public */
 export function aggregateAccessor(n: ArrayEntry): number {
   return entryValue(n)[AGGREGATE_KEY];
 }
+
 /** @public */
 export function parentAccessor(n: ArrayEntry): ArrayNode {
   return entryValue(n)[PARENT_KEY];
 }
+
 /** @public */
 export function childrenAccessor(n: ArrayEntry) {
   return entryValue(n)[CHILDREN_KEY];
 }
+
 /** @public */
 export function sortIndexAccessor(n: ArrayEntry) {
   return entryValue(n)[SORT_INDEX_KEY];
 }
+
 /** @public */
 export function pathAccessor(n: ArrayEntry) {
   return entryValue(n)[PATH_KEY];
@@ -185,7 +207,11 @@ function getRootArrayNode(): ArrayNode {
 }
 
 /** @internal */
-export function mapsToArrays(root: HierarchyOfMaps, sortSpecs: (NodeSorter | null)[]): HierarchyOfArrays {
+export function mapsToArrays(
+  root: HierarchyOfMaps,
+  sortSpecs: (NodeSorter | null)[],
+  innerGroups: LegendPath,
+): HierarchyOfArrays {
   const groupByMap = (node: HierarchyOfMaps, parent: ArrayNode) => {
     const items = Array.from(
       node,
@@ -230,7 +256,7 @@ export function mapsToArrays(root: HierarchyOfMaps, sortSpecs: (NodeSorter | nul
     mapNode[PATH_KEY] = newPath; // in-place mutation, so disabled `no-param-reassign`
     mapNode.children.forEach((entry) => buildPaths(entry, newPath));
   };
-  buildPaths(tree[0], []);
+  buildPaths(tree[0], innerGroups);
   return tree;
 }
 

diff --git a/...kages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.test.ts b/...kages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.test.ts
@@ -32,7 +32,7 @@ const groupByRollupAccessors = [() => null, (d: any) => d.sitc1];
 
 describe('Test', () => {
   test('getHierarchyOfArrays should omit zero and negative values', () => {
-    const outerResult = getHierarchyOfArrays(rawFacts, valueAccessor, groupByRollupAccessors, []);
+    const outerResult = getHierarchyOfArrays(rawFacts, valueAccessor, groupByRollupAccessors, [], []);
     expect(outerResult.length).toBe(1);
 
     const results = outerResult[0];

diff --git a/...s/packages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.ts b/...s/packages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.ts
@@ -20,6 +20,7 @@
 import { LegendItemExtraValues } from '../../../../common/legend';
 import { SeriesKey } from '../../../../common/series_id';
 import { Relation } from '../../../../common/text_utils';
+import { LegendPath } from '../../../../state/actions/legend';
 import { IndexedAccessorFn } from '../../../../utils/accessor';
 import { Datum, ValueAccessor, ValueFormatter } from '../../../../utils/common';
 import { Layer } from '../../specs';
@@ -60,6 +61,7 @@ export function getHierarchyOfArrays(
   valueAccessor: ValueAccessor,
   groupByRollupAccessors: IndexedAccessorFn[],
   sortSpecs: (NodeSorter | null)[],
+  innerGroups: LegendPath,
 ): HierarchyOfArrays {
   const aggregator = aggregators.sum;
 
@@ -76,7 +78,7 @@ export function getHierarchyOfArrays(
   // We can precompute things invariant of how the rectangle is divvied up.
   // By introducing `scale`, we no longer need to deal with the dichotomy of
   // size as data value vs size as number of pixels in the rectangle
-  return mapsToArrays(groupByRollup(groupByRollupAccessors, valueAccessor, aggregator, facts), sortSpecs);
+  return mapsToArrays(groupByRollup(groupByRollupAccessors, valueAccessor, aggregator, facts), sortSpecs, innerGroups);
 }
 
 const sorter = (layout: PartitionLayout) => ({ sortPredicate }: Layer, i: number) =>
@@ -96,13 +98,15 @@ export function partitionTree(
   layers: Layer[],
   defaultLayout: PartitionLayout,
   partitionLayout: PartitionLayout = defaultLayout,
+  innerGroups: LegendPath,
 ) {
   return getHierarchyOfArrays(
     data,
     valueAccessor,
     // eslint-disable-next-line no-shadow
     [() => HIERARCHY_ROOT_KEY, ...layers.map(({ groupByRollup }) => groupByRollup)],
     [null, ...layers.map(sorter(partitionLayout))],
+    innerGroups,
   );
 }
 

diff --git a/.../osd-charts/packages/charts/src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts b/.../osd-charts/packages/charts/src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts
@@ -262,7 +262,7 @@ const rawChildNodes = (
       const icicleLayout = isIcicle(partitionLayout);
       const icicleValueToAreaScale = width / totalValue;
       const icicleAreaAccessor = (e: ArrayEntry) => icicleValueToAreaScale * mapEntryValue(e);
-      const icicleRowHeight = height / maxDepth;
+      const icicleRowHeight = height / (maxDepth - 1);
       const result = sunburst(tree, icicleAreaAccessor, { x0: 0, y0: -icicleRowHeight }, true, false, icicleRowHeight);
       return icicleLayout
         ? result

diff --git a/packages/osd-charts/packages/charts/src/chart_types/partition_chart/partition.test.tsx b/packages/osd-charts/packages/charts/src/chart_types/partition_chart/partition.test.tsx
@@ -23,7 +23,7 @@ import { MockGlobalSpec, MockSeriesSpec } from '../../mocks/specs';
 import { MockStore } from '../../mocks/store';
 import { GlobalChartState } from '../../state/chart_state';
 import { LegendItemLabel } from '../../state/selectors/get_legend_items_labels';
-import { HIERARCHY_ROOT_KEY } from './layout/utils/group_by_rollup';
+import { HIERARCHY_ROOT_KEY, NULL_SMALL_MULTIPLES_KEY } from './layout/utils/group_by_rollup';
 import { computeLegendSelector } from './state/selectors/compute_legend';
 import { getLegendItemsLabels } from './state/selectors/get_legend_items_labels';
 
@@ -136,6 +136,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'A',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 0, value: 'A' },
           ],
@@ -148,6 +149,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'A',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 0, value: 'A' },
             { index: 0, value: 'A' },
@@ -161,6 +163,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'B',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 0, value: 'A' },
             { index: 1, value: 'B' },
@@ -174,6 +177,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'B',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 1, value: 'B' },
           ],
@@ -186,6 +190,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'A',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 1, value: 'B' },
             { index: 0, value: 'A' },
@@ -199,6 +204,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'B',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 1, value: 'B' },
             { index: 1, value: 'B' },
@@ -212,6 +218,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'C',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 2, value: 'C' },
           ],
@@ -224,6 +231,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'A',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 2, value: 'C' },
             { index: 0, value: 'A' },
@@ -237,6 +245,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'B',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             { index: 0, value: HIERARCHY_ROOT_KEY },
             { index: 2, value: 'C' },
             { index: 1, value: 'B' },
@@ -262,6 +271,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'A',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             {
               index: 0,
               value: HIERARCHY_ROOT_KEY,
@@ -280,6 +290,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'A',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             {
               index: 0,
               value: HIERARCHY_ROOT_KEY,
@@ -315,6 +326,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'C',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             {
               index: 0,
               value: HIERARCHY_ROOT_KEY,
@@ -333,6 +345,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
           childId: 'B',
           color: 'rgba(128, 0, 0, 0.5)',
           path: [
+            { index: 0, value: NULL_SMALL_MULTIPLES_KEY },
             {
               index: 0,
               value: HIERARCHY_ROOT_KEY,

diff --git a/...ges/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.test.ts b/...ges/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.test.ts
@@ -69,25 +69,25 @@ describe('Partition - Legend item extra values', () => {
 
     const extraValues = getLegendItemsExtra(store.getState());
     expect([...extraValues.keys()]).toEqual([
-      '0',
       '0__0',
       '0__0__0',
       '0__0__0__0',
+      '0__0__0__0__0',
+      '0__0__0__0__1',
       '0__0__0__1',
+      '0__0__0__1__0',
+      '0__0__0__1__1',
+      '0__0__0__1__2',
       '0__0__1',
       '0__0__1__0',
+      '0__0__1__0__0',
+      '0__0__1__0__1',
       '0__0__1__1',
+      '0__0__1__1__0',
+      '0__0__1__1__1',
       '0__0__1__2',
-      '0__1',
-      '0__1__0',
-      '0__1__0__0',
-      '0__1__0__1',
-      '0__1__1',
-      '0__1__1__0',
-      '0__1__1__1',
-      '0__1__2',
-      '0__1__2__0',
-      '0__1__2__1',
+      '0__0__1__2__0',
+      '0__0__1__2__1',
     ]);
     expect(extraValues.values()).toMatchSnapshot();
   });
@@ -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']);
     expect(extraValues.values()).toMatchSnapshot();
   });
 
@@ -107,14 +107,14 @@ describe('Partition - Legend item extra values', () => {
 
     const extraValues = getLegendItemsExtra(store.getState());
     expect([...extraValues.keys()]).toEqual([
-      '0',
       '0__0',
       '0__0__0',
+      '0__0__0__0',
+      '0__0__0__1',
       '0__0__1',
-      '0__1',
-      '0__1__0',
-      '0__1__1',
-      '0__1__2',
+      '0__0__1__0',
+      '0__0__1__1',
+      '0__0__1__2',
     ]);
     expect(extraValues.values()).toMatchSnapshot();
   });

diff --git a/...packages/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.ts b/...packages/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.ts
@@ -31,8 +31,15 @@ import { getTrees } from './tree';
 export const getLegendItemsExtra = createCachedSelector(
   [getPartitionSpec, getSettingsSpecSelector, getTrees],
   (spec, { legendMaxDepth }, trees): Map<SeriesKey, LegendItemExtraValues> => {
+    const emptyMap = new Map<SeriesKey, LegendItemExtraValues>();
     return spec && !Number.isNaN(legendMaxDepth) && legendMaxDepth > 0
-      ? getExtraValueMap(spec.layers, spec.valueFormatter, trees[0].tree, legendMaxDepth) // singleton! wrt inner small multiples
-      : new Map<SeriesKey, LegendItemExtraValues>();
+      ? trees.reduce((result, { tree }) => {
+          const treeData = getExtraValueMap(spec.layers, spec.valueFormatter, tree, legendMaxDepth);
+          for (const [key, value] of treeData) {
+            result.set(key, value);
+          }
+          return result;
+        }, emptyMap)
+      : emptyMap;
   },
 )(getChartIdSelector);