Skip to content

Commit 100145b

Browse files
monferamonfera
authored
fix(partition): getLegendItemsExtra no longer assumes a singleton (#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`
1 parent 0c31d8c commit 100145b

13 files changed

Lines changed: 120 additions & 41 deletions

File tree

‎.github/workflows/api_extractor_check.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ jobs:
2020
if: ${{ failure() }}
2121
uses: LouisBrunner/diff-action@v0.1.0
2222
with:
23-
old: api/charts.api.md
24-
new: tmp/charts.api.md
23+
old: packages/charts/api/charts.api.md
24+
new: packages/charts/tmp/charts.api.md
2525
mode: deletion
2626
tolerance: better

‎packages/charts/api/charts.api.md‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1027,6 +1027,9 @@ export interface HeatmapSpec extends Spec {
10271027
ySortPredicate: Predicate;
10281028
}
10291029

1030+
// @public
1031+
export const HIERARCHY_ROOT_KEY: Key;
1032+
10301033
// @public (undocumented)
10311034
export type HierarchyOfArrays = Array<ArrayEntry>;
10321035

@@ -1132,7 +1135,7 @@ export interface LegendColorPickerProps {
11321135
// @public (undocumented)
11331136
export type LegendItemListener = (series: SeriesIdentifier[]) => void;
11341137

1135-
// @public (undocumented)
1138+
// @public
11361139
export type LegendPath = LegendPathElement[];
11371140

11381141
// @public (undocumented)
@@ -1325,6 +1328,9 @@ export type NodeSorter = (a: ArrayEntry, b: ArrayEntry) => number;
13251328
// @public (undocumented)
13261329
export type NonAny = number | boolean | string | symbol | null;
13271330

1331+
// @public
1332+
export const NULL_SMALL_MULTIPLES_KEY: Key;
1333+
13281334
// @public (undocumented)
13291335
export interface Opacity {
13301336
opacity: number;
@@ -1528,7 +1534,7 @@ export interface Postfixes {
15281534
y1AccessorFormat?: string;
15291535
}
15301536

1531-
// @public (undocumented)
1537+
// @public
15321538
export type PrimitiveValue = string | number | null;
15331539

15341540
// @public

‎packages/charts/src/chart_types/partition_chart/layout/utils/group_by_rollup.ts‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ export interface NodeDescriptor {
5656
export type ArrayEntry = [Key, ArrayNode];
5757
/** @public */
5858
export type HierarchyOfArrays = Array<ArrayEntry>;
59+
5960
/** @public */
6061
export interface ArrayNode extends NodeDescriptor {
6162
[CHILDREN_KEY]: HierarchyOfArrays;
@@ -65,15 +66,30 @@ export interface ArrayNode extends NodeDescriptor {
6566
}
6667

6768
type HierarchyOfMaps = Map<Key, MapNode>;
69+
6870
interface MapNode extends NodeDescriptor {
6971
[CHILDREN_KEY]?: HierarchyOfMaps;
7072
[PARENT_KEY]?: ArrayNode;
7173
}
7274

73-
/** @internal */
75+
/**
76+
* Used in the first position of a `LegendPath` array, which indicates the stringified value of the `groupBy` value
77+
* in case of small multiples, but has no applicable `groupBy` for singleton (non-small-multiples) charts
78+
* @public
79+
*/
80+
export const NULL_SMALL_MULTIPLES_KEY: Key = '__null_small_multiples_key__';
81+
82+
/**
83+
* Indicates that a node is the root of a specific partition chart, eg. the root of a single pie chart, or one pie
84+
* chart in a small multiples setting. Used in the second position of a `LegendPath` array
85+
* @public
86+
*/
7487
export const HIERARCHY_ROOT_KEY: Key = '__root_key__';
7588

76-
/** @public */
89+
/**
90+
* A primitive JavaScript value, possibly further restricted
91+
* @public
92+
*/
7793
export type PrimitiveValue = string | number | null; // there could be more but sufficient for now
7894
/** @public */
7995
export type Key = CategoryKey;
@@ -90,26 +106,32 @@ export type NodeSorter = (a: ArrayEntry, b: ArrayEntry) => number;
90106
export const entryKey = ([key]: ArrayEntry) => key;
91107
/** @public */
92108
export const entryValue = ([, value]: ArrayEntry) => value;
109+
93110
/** @public */
94111
export function depthAccessor(n: ArrayEntry) {
95112
return entryValue(n)[DEPTH_KEY];
96113
}
114+
97115
/** @public */
98116
export function aggregateAccessor(n: ArrayEntry): number {
99117
return entryValue(n)[AGGREGATE_KEY];
100118
}
119+
101120
/** @public */
102121
export function parentAccessor(n: ArrayEntry): ArrayNode {
103122
return entryValue(n)[PARENT_KEY];
104123
}
124+
105125
/** @public */
106126
export function childrenAccessor(n: ArrayEntry) {
107127
return entryValue(n)[CHILDREN_KEY];
108128
}
129+
109130
/** @public */
110131
export function sortIndexAccessor(n: ArrayEntry) {
111132
return entryValue(n)[SORT_INDEX_KEY];
112133
}
134+
113135
/** @public */
114136
export function pathAccessor(n: ArrayEntry) {
115137
return entryValue(n)[PATH_KEY];
@@ -185,7 +207,11 @@ function getRootArrayNode(): ArrayNode {
185207
}
186208

187209
/** @internal */
188-
export function mapsToArrays(root: HierarchyOfMaps, sortSpecs: (NodeSorter | null)[]): HierarchyOfArrays {
210+
export function mapsToArrays(
211+
root: HierarchyOfMaps,
212+
sortSpecs: (NodeSorter | null)[],
213+
innerGroups: LegendPath,
214+
): HierarchyOfArrays {
189215
const groupByMap = (node: HierarchyOfMaps, parent: ArrayNode) => {
190216
const items = Array.from(
191217
node,
@@ -230,7 +256,7 @@ export function mapsToArrays(root: HierarchyOfMaps, sortSpecs: (NodeSorter | nul
230256
mapNode[PATH_KEY] = newPath; // in-place mutation, so disabled `no-param-reassign`
231257
mapNode.children.forEach((entry) => buildPaths(entry, newPath));
232258
};
233-
buildPaths(tree[0], []);
259+
buildPaths(tree[0], innerGroups);
234260
return tree;
235261
}
236262

‎packages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ const groupByRollupAccessors = [() => null, (d: any) => d.sitc1];
3232

3333
describe('Test', () => {
3434
test('getHierarchyOfArrays should omit zero and negative values', () => {
35-
const outerResult = getHierarchyOfArrays(rawFacts, valueAccessor, groupByRollupAccessors, []);
35+
const outerResult = getHierarchyOfArrays(rawFacts, valueAccessor, groupByRollupAccessors, [], []);
3636
expect(outerResult.length).toBe(1);
3737

3838
const results = outerResult[0];

‎packages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import { LegendItemExtraValues } from '../../../../common/legend';
2121
import { SeriesKey } from '../../../../common/series_id';
2222
import { Relation } from '../../../../common/text_utils';
23+
import { LegendPath } from '../../../../state/actions/legend';
2324
import { IndexedAccessorFn } from '../../../../utils/accessor';
2425
import { Datum, ValueAccessor, ValueFormatter } from '../../../../utils/common';
2526
import { Layer } from '../../specs';
@@ -60,6 +61,7 @@ export function getHierarchyOfArrays(
6061
valueAccessor: ValueAccessor,
6162
groupByRollupAccessors: IndexedAccessorFn[],
6263
sortSpecs: (NodeSorter | null)[],
64+
innerGroups: LegendPath,
6365
): HierarchyOfArrays {
6466
const aggregator = aggregators.sum;
6567

@@ -76,7 +78,7 @@ export function getHierarchyOfArrays(
7678
// We can precompute things invariant of how the rectangle is divvied up.
7779
// By introducing `scale`, we no longer need to deal with the dichotomy of
7880
// size as data value vs size as number of pixels in the rectangle
79-
return mapsToArrays(groupByRollup(groupByRollupAccessors, valueAccessor, aggregator, facts), sortSpecs);
81+
return mapsToArrays(groupByRollup(groupByRollupAccessors, valueAccessor, aggregator, facts), sortSpecs, innerGroups);
8082
}
8183

8284
const sorter = (layout: PartitionLayout) => ({ sortPredicate }: Layer, i: number) =>
@@ -96,13 +98,15 @@ export function partitionTree(
9698
layers: Layer[],
9799
defaultLayout: PartitionLayout,
98100
partitionLayout: PartitionLayout = defaultLayout,
101+
innerGroups: LegendPath,
99102
) {
100103
return getHierarchyOfArrays(
101104
data,
102105
valueAccessor,
103106
// eslint-disable-next-line no-shadow
104107
[() => HIERARCHY_ROOT_KEY, ...layers.map(({ groupByRollup }) => groupByRollup)],
105108
[null, ...layers.map(sorter(partitionLayout))],
109+
innerGroups,
106110
);
107111
}
108112

‎packages/charts/src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ const rawChildNodes = (
262262
const icicleLayout = isIcicle(partitionLayout);
263263
const icicleValueToAreaScale = width / totalValue;
264264
const icicleAreaAccessor = (e: ArrayEntry) => icicleValueToAreaScale * mapEntryValue(e);
265-
const icicleRowHeight = height / maxDepth;
265+
const icicleRowHeight = height / (maxDepth - 1);
266266
const result = sunburst(tree, icicleAreaAccessor, { x0: 0, y0: -icicleRowHeight }, true, false, icicleRowHeight);
267267
return icicleLayout
268268
? result

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import { MockGlobalSpec, MockSeriesSpec } from '../../mocks/specs';
2323
import { MockStore } from '../../mocks/store';
2424
import { GlobalChartState } from '../../state/chart_state';
2525
import { LegendItemLabel } from '../../state/selectors/get_legend_items_labels';
26-
import { HIERARCHY_ROOT_KEY } from './layout/utils/group_by_rollup';
26+
import { HIERARCHY_ROOT_KEY, NULL_SMALL_MULTIPLES_KEY } from './layout/utils/group_by_rollup';
2727
import { computeLegendSelector } from './state/selectors/compute_legend';
2828
import { getLegendItemsLabels } from './state/selectors/get_legend_items_labels';
2929

@@ -136,6 +136,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
136136
childId: 'A',
137137
color: 'rgba(128, 0, 0, 0.5)',
138138
path: [
139+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
139140
{ index: 0, value: HIERARCHY_ROOT_KEY },
140141
{ index: 0, value: 'A' },
141142
],
@@ -148,6 +149,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
148149
childId: 'A',
149150
color: 'rgba(128, 0, 0, 0.5)',
150151
path: [
152+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
151153
{ index: 0, value: HIERARCHY_ROOT_KEY },
152154
{ index: 0, value: 'A' },
153155
{ index: 0, value: 'A' },
@@ -161,6 +163,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
161163
childId: 'B',
162164
color: 'rgba(128, 0, 0, 0.5)',
163165
path: [
166+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
164167
{ index: 0, value: HIERARCHY_ROOT_KEY },
165168
{ index: 0, value: 'A' },
166169
{ index: 1, value: 'B' },
@@ -174,6 +177,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
174177
childId: 'B',
175178
color: 'rgba(128, 0, 0, 0.5)',
176179
path: [
180+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
177181
{ index: 0, value: HIERARCHY_ROOT_KEY },
178182
{ index: 1, value: 'B' },
179183
],
@@ -186,6 +190,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
186190
childId: 'A',
187191
color: 'rgba(128, 0, 0, 0.5)',
188192
path: [
193+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
189194
{ index: 0, value: HIERARCHY_ROOT_KEY },
190195
{ index: 1, value: 'B' },
191196
{ index: 0, value: 'A' },
@@ -199,6 +204,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
199204
childId: 'B',
200205
color: 'rgba(128, 0, 0, 0.5)',
201206
path: [
207+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
202208
{ index: 0, value: HIERARCHY_ROOT_KEY },
203209
{ index: 1, value: 'B' },
204210
{ index: 1, value: 'B' },
@@ -212,6 +218,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
212218
childId: 'C',
213219
color: 'rgba(128, 0, 0, 0.5)',
214220
path: [
221+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
215222
{ index: 0, value: HIERARCHY_ROOT_KEY },
216223
{ index: 2, value: 'C' },
217224
],
@@ -224,6 +231,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
224231
childId: 'A',
225232
color: 'rgba(128, 0, 0, 0.5)',
226233
path: [
234+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
227235
{ index: 0, value: HIERARCHY_ROOT_KEY },
228236
{ index: 2, value: 'C' },
229237
{ index: 0, value: 'A' },
@@ -237,6 +245,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
237245
childId: 'B',
238246
color: 'rgba(128, 0, 0, 0.5)',
239247
path: [
248+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
240249
{ index: 0, value: HIERARCHY_ROOT_KEY },
241250
{ index: 2, value: 'C' },
242251
{ index: 1, value: 'B' },
@@ -262,6 +271,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
262271
childId: 'A',
263272
color: 'rgba(128, 0, 0, 0.5)',
264273
path: [
274+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
265275
{
266276
index: 0,
267277
value: HIERARCHY_ROOT_KEY,
@@ -280,6 +290,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
280290
childId: 'A',
281291
color: 'rgba(128, 0, 0, 0.5)',
282292
path: [
293+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
283294
{
284295
index: 0,
285296
value: HIERARCHY_ROOT_KEY,
@@ -315,6 +326,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
315326
childId: 'C',
316327
color: 'rgba(128, 0, 0, 0.5)',
317328
path: [
329+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
318330
{
319331
index: 0,
320332
value: HIERARCHY_ROOT_KEY,
@@ -333,6 +345,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
333345
childId: 'B',
334346
color: 'rgba(128, 0, 0, 0.5)',
335347
path: [
348+
{ index: 0, value: NULL_SMALL_MULTIPLES_KEY },
336349
{
337350
index: 0,
338351
value: HIERARCHY_ROOT_KEY,

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

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -69,25 +69,25 @@ describe('Partition - Legend item extra values', () => {
6969

7070
const extraValues = getLegendItemsExtra(store.getState());
7171
expect([...extraValues.keys()]).toEqual([
72-
'0',
7372
'0__0',
7473
'0__0__0',
7574
'0__0__0__0',
75+
'0__0__0__0__0',
76+
'0__0__0__0__1',
7677
'0__0__0__1',
78+
'0__0__0__1__0',
79+
'0__0__0__1__1',
80+
'0__0__0__1__2',
7781
'0__0__1',
7882
'0__0__1__0',
83+
'0__0__1__0__0',
84+
'0__0__1__0__1',
7985
'0__0__1__1',
86+
'0__0__1__1__0',
87+
'0__0__1__1__1',
8088
'0__0__1__2',
81-
'0__1',
82-
'0__1__0',
83-
'0__1__0__0',
84-
'0__1__0__1',
85-
'0__1__1',
86-
'0__1__1__0',
87-
'0__1__1__1',
88-
'0__1__2',
89-
'0__1__2__0',
90-
'0__1__2__1',
89+
'0__0__1__2__0',
90+
'0__0__1__2__1',
9191
]);
9292
expect(extraValues.values()).toMatchSnapshot();
9393
});
@@ -97,7 +97,7 @@ describe('Partition - Legend item extra values', () => {
9797
MockStore.addSpecs([settings, spec], store);
9898

9999
const extraValues = getLegendItemsExtra(store.getState());
100-
expect([...extraValues.keys()]).toEqual(['0', '0__0', '0__1']);
100+
expect([...extraValues.keys()]).toEqual(['0__0', '0__0__0', '0__0__1']);
101101
expect(extraValues.values()).toMatchSnapshot();
102102
});
103103

@@ -107,14 +107,14 @@ describe('Partition - Legend item extra values', () => {
107107

108108
const extraValues = getLegendItemsExtra(store.getState());
109109
expect([...extraValues.keys()]).toEqual([
110-
'0',
111110
'0__0',
112111
'0__0__0',
112+
'0__0__0__0',
113+
'0__0__0__1',
113114
'0__0__1',
114-
'0__1',
115-
'0__1__0',
116-
'0__1__1',
117-
'0__1__2',
115+
'0__0__1__0',
116+
'0__0__1__1',
117+
'0__0__1__2',
118118
]);
119119
expect(extraValues.values()).toMatchSnapshot();
120120
});

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,15 @@ import { getTrees } from './tree';
3131
export const getLegendItemsExtra = createCachedSelector(
3232
[getPartitionSpec, getSettingsSpecSelector, getTrees],
3333
(spec, { legendMaxDepth }, trees): Map<SeriesKey, LegendItemExtraValues> => {
34+
const emptyMap = new Map<SeriesKey, LegendItemExtraValues>();
3435
return spec && !Number.isNaN(legendMaxDepth) && legendMaxDepth > 0
35-
? getExtraValueMap(spec.layers, spec.valueFormatter, trees[0].tree, legendMaxDepth) // singleton! wrt inner small multiples
36-
: new Map<SeriesKey, LegendItemExtraValues>();
36+
? trees.reduce((result, { tree }) => {
37+
const treeData = getExtraValueMap(spec.layers, spec.valueFormatter, tree, legendMaxDepth);
38+
for (const [key, value] of treeData) {
39+
result.set(key, value);
40+
}
41+
return result;
42+
}, emptyMap)
43+
: emptyMap;
3744
},
3845
)(getChartIdSelector);

0 commit comments

Comments
 (0)