Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 7 commits into from
Jun 10, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bfbcd08
fix: getLegendItemsExtra no longer assumes a singleton
monfera Jun 8, 2021
46f13d6
fix: icicle and flame should ignore the grouping
monfera Jun 9, 2021
ef40b2d
Merge remote-tracking branch 'origin/master' into singleton-partition…
monfera Jun 9, 2021
874b94a
chore: remove disused function
monfera Jun 10, 2021
da812fa
refactor: exported `HIERARCHY_ROOT_KEY` and also added `NULL_SMALL_MU…
monfera Jun 10, 2021
afc546b
chore: api update and api generator path update
monfera Jun 10, 2021
d7a18fd
Merge remote-tracking branch 'origin/master' into singleton-partition…
monfera Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/api_extractor_check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:
if: ${{ failure() }}
uses: LouisBrunner/diff-action@v0.1.0
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
10 changes: 8 additions & 2 deletions packages/charts/api/charts.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;

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

// @public (undocumented)
// @public
export type LegendPath = LegendPathElement[];

// @public (undocumented)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1528,7 +1534,7 @@ export interface Postfixes {
y1AccessorFormat?: string;
}

// @public (undocumented)
// @public
export type PrimitiveValue = string | number | null;

// @public
Expand Down
34 changes: 30 additions & 4 deletions packages/charts/src/chart_types/partition_chart/layout/utils/group_by_rollup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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];
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
6 changes: 5 additions & 1 deletion packages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -60,6 +61,7 @@ export function getHierarchyOfArrays(
valueAccessor: ValueAccessor,
groupByRollupAccessors: IndexedAccessorFn[],
sortSpecs: (NodeSorter | null)[],
innerGroups: LegendPath,
): HierarchyOfArrays {
const aggregator = aggregators.sum;

Expand All @@ -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) =>
Expand All @@ -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,
);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/charts/src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 14 additions & 1 deletion packages/charts/src/chart_types/partition_chart/partition.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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' },
],
Expand All @@ -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' },
Expand All @@ -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' },
Expand All @@ -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' },
],
Expand All @@ -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' },
Expand All @@ -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' },
Expand All @@ -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' },
],
Expand All @@ -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' },
Expand All @@ -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' },
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
34 changes: 17 additions & 17 deletions ...ges/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand All @@ -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']);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@monfera monfera Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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, .....]

expect(extraValues.values()).toMatchSnapshot();
});

Expand All @@ -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();
});
Expand Down
11 changes: 9 additions & 2 deletions packages/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Loading