Skip to content

Commit

Permalink
fix(legend): hierarchical legend order should follow the tree paths (o…
Browse files Browse the repository at this point in the history 
…pensearch-project#947)

refactor: saving the tree path information in the tree nodes; legend item sorting based on the path; code simplifications; minor perf improvements

fix opensearch-project#944
  • Loading branch information
@monfera
monfera authored Dec 16, 2020
1 parent dfff3e2 commit 7b70186
Show file tree
Hide file tree
Showing 22 changed files with 450 additions and 276 deletions.
3 changes: 2 additions & 1 deletion packages/osd-charts/.eslintrc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ module.exports = {
],
rules: {
/**
* depricated to be deleted
* deprecated to be deleted
*/
// https://github.com/typescript-eslint/typescript-eslint/issues/2077
'@typescript-eslint/camelcase': 0,
Expand Down Expand Up @@ -92,6 +92,7 @@ module.exports = {
/**
* Standard rules
*/
'no-restricted-syntax': 0, // this is a good rule, for-of is good
'no-console': process.env.NODE_ENV === 'production' ? 2 : 1,
'no-debugger': process.env.NODE_ENV === 'production' ? 2 : 1,
'prefer-template': 'error',
Expand Down
189 changes: 50 additions & 139 deletions packages/osd-charts/.playground/playground.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,149 +17,60 @@
* under the License.
*/

import React from 'react';

interface Food {
label: string;
count: number;
actionLabel: string;
}
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

type Foods = Array<Food>;
import React from 'react';

type FoodArray = Array<string>;
import { Chart, Settings, Partition, PartitionLayout } from '../src';

export class Playground extends React.Component {
foods: Foods = [
{ label: 'pie', count: 2, actionLabel: 'tab' },
{ label: 'asparagus', count: 5, actionLabel: 'tab' },
{ label: 'brownies', count: 0, actionLabel: 'enter' },
{ label: 'popsicles', count: 3, actionLabel: 'enter' },
];

foodsAsAnArray: FoodArray = ['tab', 'tab', 'tab', 'enter'];

getFoodsArrayAction = (foodsArray: FoodArray) => {
for (let i = 0; i < foodsArray.length; i++) {
if (foodsArray[i] === 'tab') {
// alert('tab!');
} else if (foodsArray[i] === 'enter') {
// alert('enter!');
}
}
};

getFoodAction = (foodLabel: Food[keyof Food]) => {
// eslint-disable-next-line array-callback-return
return this.foods.map(({ label, count, actionLabel }) => {
if (foodLabel === label && actionLabel === 'tab') {
let c = 0;
while (c < count) {
// alert(`${label} Tab!`);
c++;
}
} else if (foodLabel === label && actionLabel === 'enter') {
let c = 0;
while (c < count) {
// alert(`${label} Enter!`);
c++;
}
}
});
};

makingFood = (ms: number, food: string) => {
return new Promise((resolve) => {
setTimeout(() => {
// console.log(`resolving the promise ${food}`, new Date());
resolve(`done with ${food}`);
}, ms);
// console.log(`starting the promise ${food}`, new Date());
});
};

getNumberOfFood = (food: any) => {
return this.makingFood(1000, food).then(() => this.getFoodAction(food));
};

getNumberOfFoodArray = () => {
return this.makingFood(1000, 'apple').then(() => this.getFoodsArrayAction(this.foodsAsAnArray));
};

getAsyncNumberOfFoodArray = async () => {
// const result = await this.makingFood(2000).then(() => this.getFoodsArrayAction(this.foodsAsAnArray));
// alert(result);
};

// forLoop = async () => {
// alert('start');
// for (let index = 0; index < this.foods.length; index++) {
// const foodLabel = this.foods[index].label;
// const numFood = await this.getFoodNumber(foodLabel);
// alert(numFood);
// }
// const foodsPromiseArray = this.foods.map(async (foodObject) => {
// for (let i = 0; i < foodObject.length; i++) {
// const numFoodAction = foodObject[i].actionLabel;
// if (numFoodAction === 'enter') {
// alert ('Enter!');
// } else if (numFoodAction === 'tab') {
// alert('tab!');
// }
// }
// });
// const numberOfFoods = await Promise.all(foodsPromiseArray);
// alert(numberOfFoods);
// alert('End');
// };

// getFoodArray =
// async() => {
// console.log(await this.makingFood(1000, 'apricot'));
// console.log(await this.makingFood(50, 'apple'));
// const foodTimeArray = [
// { ms: 1000, food: 'a', count: 2 },
// { ms: 50, food: 'b', count: 1 },
// { ms: 500, food: 'c', count: 3 },
// ];

// for (let i = 0; i < foodTimeArray.length; i++) {
// void this.makingFood(foodTimeArray[i].ms, foodTimeArray[i].food);
// }

// const foodMap = foodTimeArray.map(({ ms, food }) => {
// return this.makingFood(ms, food);
// });

// console.log('before the promise');
// for (const i of foodTimeArray) {
// const j = 0;
// while (j < i.count) {
// await this.makingFood(i.ms, i.food);
// j++;
// }
// }

// await Promise.all();
// console.log('after the promise');
// };

render() {
// console.log(this.makingFood(1000, 'apricot'));
// console.log(this.makingFood(50, 'apple'));
// void this.getFoodArray();

return null;
// <>
// <div className="page" style={{ width: 5000, height: 5000, backgroundColor: 'yellow' }}>
// <div id="root" style={{ backgroundColor: 'blueviolet' }}>
// {/* <div>{this.foods.map(({ label }) => this.getNumberOfFood(label))}</div> */}
// {/* <div>{alert(this.makingFood(50000).then(this.getFoodsArrayAction(this.foodsAsAnArray)))}</div> */}
// {/* <div>{alert(this.getNumberOfFoodArray())}</div> */}
// {/* <div>{alert(this.getAsyncNumberOfFoodArray())}</div> */}
// <div>{this.makingFood(50)}</div>
// </div>
// </div>
// </>
return (
<div className="chart">
<Chart className="story-chart">
<Settings showLegend flatLegend={false} />
<Partition
id="spec_1"
data={[
{ cat1: 'A', cat2: 'A', val: 1 },
{ cat1: 'A', cat2: 'B', val: 1 },
{ cat1: 'B', cat2: 'A', val: 1 },
{ cat1: 'B', cat2: 'B', val: 1 },
{ cat1: 'C', cat2: 'A', val: 1 },
{ cat1: 'C', cat2: 'B', val: 1 },
]}
valueAccessor={(d: any) => d.val as number}
layers={[
{
groupByRollup: (d: any) => d.cat1,
},
{
groupByRollup: (d: any) => d.cat2,
},
]}
config={{
partitionLayout: PartitionLayout.sunburst,
}}
/>
</Chart>
</div>
);
}
}
4 changes: 2 additions & 2 deletions packages/osd-charts/api/charts.api.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ export const DEFAULT_TOOLTIP_TYPE: "vertical";
// Warning: (ae-missing-release-tag) "DefaultSettingsProps" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export type DefaultSettingsProps = 'id' | 'chartType' | 'specType' | 'rendering' | 'rotation' | 'resizeDebounce' | 'animateData' | 'showLegend' | 'debug' | 'tooltip' | 'showLegendExtra' | 'theme' | 'legendPosition' | 'hideDuplicateAxes' | 'brushAxis' | 'minBrushDelta' | 'externalPointerEvents';
export type DefaultSettingsProps = 'id' | 'chartType' | 'specType' | 'rendering' | 'rotation' | 'resizeDebounce' | 'animateData' | 'showLegend' | 'debug' | 'tooltip' | 'showLegendExtra' | 'theme' | 'legendPosition' | 'legendMaxDepth' | 'hideDuplicateAxes' | 'brushAxis' | 'minBrushDelta' | 'externalPointerEvents';

// Warning: (ae-missing-release-tag) "Direction" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
Expand Down Expand Up @@ -1597,7 +1597,7 @@ export interface SettingsSpec extends Spec {
legendAction?: LegendAction;
// (undocumented)
legendColorPicker?: LegendColorPicker;
legendMaxDepth?: number;
legendMaxDepth: number;
legendPosition: Position;
minBrushDelta?: number;
noResults?: ComponentType | ReactChild;
Expand Down
Binary file added ...r-all-stories-legend-piechart-repeated-labels-visually-looks-correct-1-snap.png
View file
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...ame-order-as-nested-with-no-indent-even-if-there-are-repeated-labels-1-snap.png
View file
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
5 changes: 5 additions & 0 deletions packages/osd-charts/integration/tests/legend_stories.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ describe('Legend stories', () => {
'http://localhost:9001/?path=/story/legend--legend-spacing-buffer&knob-legend buffer value=0',
);
});
it('should have the same order as nested with no indent even if there are repeated labels', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/legend--piechart-repeated-labels&knob-flatLegend=true&knob-legendMaxDepth=2',
);
});

it('should render color picker on mouse click', async () => {
const action = async () =>
Expand Down
5 changes: 4 additions & 1 deletion packages/osd-charts/src/chart_types/partition_chart/layout/types/viewmodel_types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,14 @@ interface SectorGeomSpecY {
y1px: Distance;
}

export type DataName = any; // todo consider narrowing it to eg. primitives

export interface ShapeTreeNode extends TreeNode, SectorGeomSpecY {
yMidPx: Distance;
depth: number;
sortIndex: number;
dataName: any;
path: number[];
dataName: DataName;
value: number;
parent: ArrayNode;
}
Expand Down
23 changes: 21 additions & 2 deletions packages/osd-charts/src/chart_types/partition_chart/layout/utils/group_by_rollup.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const CHILDREN_KEY = 'children';
export const INPUT_KEY = 'inputIndex';
export const PARENT_KEY = 'parent';
export const SORT_INDEX_KEY = 'sortIndex';
export const PATH_KEY = 'path';

interface Statistics {
globalAggregate: number;
Expand All @@ -45,6 +46,7 @@ export interface ArrayNode extends NodeDescriptor {
[CHILDREN_KEY]: HierarchyOfArrays;
[PARENT_KEY]: ArrayNode;
[SORT_INDEX_KEY]: number;
[PATH_KEY]: number[];
}

type HierarchyOfMaps = Map<Key, MapNode>;
Expand Down Expand Up @@ -75,6 +77,9 @@ export function childrenAccessor(n: ArrayEntry) {
export function sortIndexAccessor(n: ArrayEntry) {
return entryValue(n)[SORT_INDEX_KEY];
}
export function pathAccessor(n: ArrayEntry) {
return entryValue(n)[PATH_KEY];
}
const ascending: Sorter = (a, b) => a - b;
const descending: Sorter = (a, b) => b - a;

Expand Down Expand Up @@ -128,7 +133,13 @@ export function groupByRollup(

function getRootArrayNode(): ArrayNode {
const children: HierarchyOfArrays = [];
const bootstrap = { [AGGREGATE_KEY]: NaN, [DEPTH_KEY]: NaN, [CHILDREN_KEY]: children, [INPUT_KEY]: [] as number[] };
const bootstrap = {
[AGGREGATE_KEY]: NaN,
[DEPTH_KEY]: NaN,
[CHILDREN_KEY]: children,
[INPUT_KEY]: [] as number[],
[PATH_KEY]: [] as number[],
};
Object.assign(bootstrap, { [PARENT_KEY]: bootstrap });
const result: ArrayNode = bootstrap as ArrayNode;
return result;
Expand All @@ -149,6 +160,7 @@ export function mapsToArrays(root: HierarchyOfMaps, sorter: NodeSorter): Hierarc
[SORT_INDEX_KEY]: NaN,
[PARENT_KEY]: parent,
[INPUT_KEY]: [],
[PATH_KEY]: [],
};
const newValue: ArrayNode = Object.assign(
resultNode,
Expand All @@ -163,7 +175,14 @@ export function mapsToArrays(root: HierarchyOfMaps, sorter: NodeSorter): Hierarc
entryValue(n).sortIndex = i;
return n;
}); // with the current algo, decreasing order is important
return groupByMap(root, getRootArrayNode());
const tree = groupByMap(root, getRootArrayNode());
const buildPaths = ([, mapNode]: ArrayEntry, currentPath: number[]) => {
const newPath = [...currentPath, mapNode[SORT_INDEX_KEY]];
mapNode[PATH_KEY] = newPath;
mapNode.children.forEach((entry) => buildPaths(entry, newPath));
};
buildPaths(tree[0], []);
return tree;
}

/** @internal */
Expand Down
2 changes: 2 additions & 0 deletions packages/osd-charts/src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
parentAccessor,
sortIndexAccessor,
HierarchyOfArrays,
pathAccessor,
} from '../utils/group_by_rollup';
import { trueBearingToStandardPositionAngle } from '../utils/math';
import { sunburst } from '../utils/sunburst';
Expand Down Expand Up @@ -349,6 +350,7 @@ function partToShapeTreeNode(treemapLayout: boolean, innerRadius: Radius, ringTh
value: aggregateAccessor(node),
parent: parentAccessor(node),
sortIndex: sortIndexAccessor(node),
path: pathAccessor(node),
x0,
x1,
y0,
Expand Down
Loading

0 comments on commit 7b70186

Please sign in to comment.