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.

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

refactor(partition): improving on chart events and callbacks #991

Merged
merged 15 commits into from
Jan 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .playground/playground.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import React from 'react';

import { Example } from '../stories/small_multiples/6_heterogeneous_cartesians';
import { Example } from '../stories/icicle/02_unix_flame';

export class Playground extends React.Component {
render() {
Expand Down
27 changes: 21 additions & 6 deletions api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,9 @@ export interface BubbleSeriesStyle {
point: PointStyle;
}

// @public (undocumented)
export type CategoryKey = string;

// Warning: (ae-forgotten-export) The symbol "ChartProps" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "ChartState" needs to be exported by the entry point index.d.ts
// Warning: (ae-missing-release-tag) "Chart" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
Expand Down Expand Up @@ -1021,15 +1024,12 @@ export type HorizontalAlignment = $Values<typeof HorizontalAlignment>;
// @public
export type IndexedAccessorFn = UnaryAccessorFn | BinaryAccessorFn;

// Warning: (ae-missing-release-tag) "LayerValue" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export interface LayerValue {
// Warning: (ae-forgotten-export) The symbol "PrimitiveValue" needs to be exported by the entry point index.d.ts
//
// (undocumented)
depth: number;
groupByRollup: PrimitiveValue;
// (undocumented)
path: LegendPath;
sortIndex: number;
value: number;
}

Expand Down Expand Up @@ -1064,6 +1064,15 @@ export interface LegendColorPickerProps {
// @public (undocumented)
export type LegendItemListener = (series: SeriesIdentifier | null) => void;

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

// @public (undocumented)
export type LegendPathElement = {
index: number;
value: CategoryKey;
};

// @public (undocumented)
export const LegendStrategy: Readonly<{
Node: "node";
Expand Down Expand Up @@ -1187,6 +1196,9 @@ export function mergeWithDefaultAnnotationRect(config?: Partial<RectAnnotationSt
// @public
export function mergeWithDefaultTheme(theme: PartialTheme, defaultTheme?: Theme, axillaryThemes?: PartialTheme[]): Theme;

// @public (undocumented)
export const MODEL_KEY = "parent";

// Warning: (ae-missing-release-tag) "niceTimeFormatByDay" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
Expand Down Expand Up @@ -1388,6 +1400,9 @@ export interface Postfixes {
y1AccessorFormat?: string;
}

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

// @public
export type ProjectedValues = {
x: PrimitiveValue;
Expand Down
4 changes: 2 additions & 2 deletions docs/0-Intro/1-Overview.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ If you have `textInvertible` set to true, but do not have `textContrast` set to
nodeLabel: (d) => countryLookup[d].name,
shape: {
fillColor: (d) => {
return categoricalFillColor(colorBrewerCategoricalStark9, 0.3)(d.parent.parent.sortIndex);
return categoricalFillColor(colorBrewerCategoricalStark9, 0.3)(d[MODEL_KEY].parent.sortIndex);
},
},
},
Expand Down Expand Up @@ -387,7 +387,7 @@ Now if you set the `textContrast` to true as well, these slices also become blac
nodeLabel: (d) => countryLookup[d].name,
shape: {
fillColor: (d) => {
return categoricalFillColor(colorBrewerCategoricalStark9, 0.3)(d.parent.parent.sortIndex);
return categoricalFillColor(colorBrewerCategoricalStark9, 0.3)(d[MODEL_KEY].parent.sortIndex);
},
},
},
Expand Down
3 changes: 3 additions & 0 deletions src/chart_types/goal_chart/state/selectors/picked_shapes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ export const getPickedShapesLayerValues = createCachedSelector(
values.push({
groupByRollup: 'Actual',
value: model.actual,
sortIndex: 0,
path: [],
depth: 0,
});
return values.reverse();
});
Expand Down
21 changes: 13 additions & 8 deletions src/chart_types/partition_chart/layout/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,34 @@
*/

import { Config, PartitionLayout, Numeric } from '../types/config_types';
import { FONT_STYLES, FONT_VARIANTS } from '../types/types';
import { FONT_STYLES, FONT_VARIANTS, MODEL_KEY } from '../types/types';
import { ShapeTreeNode } from '../types/viewmodel_types';
import { GOLDEN_RATIO, TAU } from '../utils/constants';
import { AGGREGATE_KEY, STATISTICS_KEY } from '../utils/group_by_rollup';

const log10 = Math.log(10);
const LOG_10 = Math.log(10);

function significantDigitCount(d: number): number {
let n = Math.abs(parseFloat(String(d).replace('.', ''))); // remove decimal and make positive
if (n == 0) return 0;
while (n != 0 && n % 10 == 0) n /= 10;
return Math.floor(Math.log(n) / log10) + 1;
let n = Math.abs(parseFloat(String(d).replace('.', '')));
if (n === 0) {
return 0;
}
while (n !== 0 && n % 10 === 0) {
n /= 10;
}
return Math.floor(Math.log(n) / LOG_10) + 1;
}

export function sumValueGetter(node: ShapeTreeNode): number {
return node[AGGREGATE_KEY];
}

export function percentValueGetter(node: ShapeTreeNode): number {
return (100 * node[AGGREGATE_KEY]) / node.parent[STATISTICS_KEY].globalAggregate;
return (100 * node[AGGREGATE_KEY]) / node[MODEL_KEY][STATISTICS_KEY].globalAggregate;
}

export function ratioValueGetter(node: ShapeTreeNode): number {
return node[AGGREGATE_KEY] / node.parent[STATISTICS_KEY].globalAggregate;
return node[AGGREGATE_KEY] / node[MODEL_KEY][STATISTICS_KEY].globalAggregate;
}

export const VALUE_GETTERS = Object.freeze({ percent: percentValueGetter, ratio: ratioValueGetter } as const);
Expand Down
15 changes: 15 additions & 0 deletions src/chart_types/partition_chart/layout/types/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,18 @@ export interface Rectangle extends Origin {
export interface Part extends Rectangle {
node: ArrayEntry;
}

/**
* It's an unfortunate accident that 'parent' is used both
* - for linking an ArrayNode to a QuadViewModel, and
* - for recursively linking the parent ArrayNode to an ArrayNode (child) in the tree
*
* By extracting out the 'MODEL_KEY', we make the distinction clear, while the API, which depends on this, doesn't
* change. This makes an eventual API change a single-line change, assuming `[MODEL_KEY]` is used where needed, and just there
*
* Todo:
* - replace users' use of `s.parent` with `s[MODEL_KEY]` for the ShapeTreeNode -> ArrayNode access
* - change MODEL_KEY to something other than 'parent' when it's done (might still be breaking change)
*/
/** @public */
export const MODEL_KEY = 'parent';
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { VerticalAlignments } from '../viewmodel/constants';
import { LinkLabelsViewModelSpec } from '../viewmodel/link_text_layout';
import { Config } from './config_types';
import { Coordinate, Distance, Pixels, PointObject, PointTuple, PointTuples, Radian } from './geometry_types';
import { Font } from './types';
import { Font, MODEL_KEY } from './types';

/** @internal */
export type LinkLabelVM = {
Expand Down Expand Up @@ -159,7 +159,7 @@ export interface ShapeTreeNode extends TreeNode, SectorGeomSpecY {
path: LegendPath;
dataName: DataName;
value: number;
parent: ArrayNode;
[MODEL_KEY]: ArrayNode;
Copy link
Collaborator

@nickofthyme nickofthyme Jan 25, 2021

Choose a reason for hiding this comment

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

That seems fine to me. Are you trying to avoid breaking changes? I'd say this would be breaking.

One way to somewhat deafen a breaking change would be to keep the parent key but add the @deprecated flag. Replace all usage in kibana with the MODEL_KEY string. Then later we could change the name but it would be a cleaner breaking change when we remove parent.

Suggested change
[MODEL_KEY]: ArrayNode;
/** @deprecated use `MODEL_KEY` instead */
parent: ArrayNode;
[MODEL_KEY]: ArrayNode;

Copy link
Contributor Author

@monfera monfera Jan 25, 2021

Choose a reason for hiding this comment

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

Thanks Nick! The plan would be:

Full compatibility stage, no breaking API, eg. after merging this PR ("whatever" changed back to "parent")

  • we still call it parent
  • everything still works and compliant with our API
  • a new value appears in the API, MODEL_KEY

Deprecation stage:

  • we and users can start replace .parent with [MODEL_KEY]
  • still compatible with the API as is; something gets a @deprecated tag, at least conceptually, as I think we can't have two identically named props at the same time, though haven't tried

Breaking change:

  • When nobody in at least Kibana uses .parent for the model, we make the breaking change of const MODEL_KEY = 'model'
  • though it's a breaking change, it'll no longer impact known users due to our assist during deprecation, while other users simply need to deal with a changing API

I'm not sure if the proposed commit would still work, pls. indicate if that's the case, glad to try. At least from the diff, it's not clear what the deprecation line belongs to

}

export type RawTextGetter = (node: ShapeTreeNode) => string;
Expand Down
14 changes: 11 additions & 3 deletions src/chart_types/partition_chart/layout/utils/group_by_rollup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import { CategoryKey } from '../../../../common/category';
import { LegendPath } from '../../../../state/actions/legend';
import { Datum } from '../../../../utils/common';
import { Datum, ValueAccessor } from '../../../../utils/common';
import { Relation } from '../types/types';

export const AGGREGATE_KEY = 'value';
Expand Down Expand Up @@ -60,6 +60,7 @@ interface MapNode extends NodeDescriptor {
/** @internal */
export const HIERARCHY_ROOT_KEY: Key = '__root_key__';

/** @public */
export type PrimitiveValue = string | number | null; // there could be more but sufficient for now
type Key = CategoryKey;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a huge fan of redefining types like this. But can be changed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, let's chat

export type Sorter = (a: number, b: number) => number;
Expand Down Expand Up @@ -88,10 +89,17 @@ export function pathAccessor(n: ArrayEntry) {
const ascending: Sorter = (a, b) => a - b;
const descending: Sorter = (a, b) => b - a;

/** @public */
export function getNodeName(node: ArrayNode) {
const index = node[SORT_INDEX_KEY];
const arrayEntry: ArrayEntry = node[PARENT_KEY][CHILDREN_KEY][index];
return entryKey(arrayEntry);
}

/** @internal */
export function groupByRollup(
keyAccessors: Array<((a: Datum) => Key) | ((a: Datum, i: number) => Key)>,
valueAccessor: (v: any) => any,
valueAccessor: ValueAccessor,
{
reducer,
identity,
Expand All @@ -108,7 +116,7 @@ export function groupByRollup(
const keyCount = keyAccessors.length;
let pointer: HierarchyOfMaps = p;
keyAccessors.forEach((keyAccessor, i) => {
const key = keyAccessor(n, index);
const key: Key = keyAccessor(n, index);
const last = i === keyCount - 1;
const node = pointer.get(key);
const inputIndices = node?.[INPUT_KEY] ?? [];
Expand Down
6 changes: 3 additions & 3 deletions src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { percentValueGetter } from '../config/config';
import { meanAngle } from '../geometry';
import { Config, PartitionLayout } from '../types/config_types';
import { Distance, Pixels, PointTuple, Radius } from '../types/geometry_types';
import { TextMeasure, Part } from '../types/types';
import { TextMeasure, Part, MODEL_KEY } from '../types/types';
import {
nullShapeViewModel,
OutsideLinksViewModel,
Expand Down Expand Up @@ -104,7 +104,7 @@ export function makeQuadViewModel(
const layer = layers[node.depth - 1];
const fillColorSpec = layer && layer.shape && layer.shape.fillColor;
const fill = fillColorSpec ?? 'rgba(128,0,0,0.5)';
const shapeFillColor = typeof fill === 'function' ? fill(node, node.sortIndex, node.parent.children) : fill;
const shapeFillColor = typeof fill === 'function' ? fill(node, node.sortIndex, node[MODEL_KEY].children) : fill;
const { r, g, b, opacity } = stringToRGB(shapeFillColor);
const fillColor = argsToRGBString(r, g, b, opacity * opacityMultiplier);
const strokeWidth = sectorLineWidth;
Expand Down Expand Up @@ -407,7 +407,7 @@ function partToShapeTreeNode(treemapLayout: boolean, innerRadius: Radius, ringTh
dataName: entryKey(node),
depth: depthAccessor(node),
value: aggregateAccessor(node),
parent: parentAccessor(node),
[MODEL_KEY]: parentAccessor(node),
sortIndex: sortIndexAccessor(node),
path: pathAccessor(node),
x0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { GlobalChartState } from '../../../../state/chart_state';
import { getChartContainerDimensionsSelector } from '../../../../state/selectors/get_chart_container_dimensions';
import { getInternalIsInitializedSelector, InitStatus } from '../../../../state/selectors/get_internal_is_intialized';
import { Dimensions } from '../../../../utils/dimensions';
import { MODEL_KEY } from '../../layout/types/types';
import { nullShapeViewModel, QuadViewModel, ShapeViewModel } from '../../layout/types/viewmodel_types';
import { INPUT_KEY } from '../../layout/utils/group_by_rollup';
import { partitionGeometries } from '../../state/selectors/geometries';
Expand Down Expand Up @@ -114,7 +115,7 @@ class PartitionComponent extends React.Component<PartitionProps> {
const pickedShapes: Array<QuadViewModel> = picker(x, y);
const datumIndices = new Set();
pickedShapes.forEach((shape) => {
const node = shape.parent;
const node = shape[MODEL_KEY];
const shapeNode = node.children.find(([key]) => key === shape.dataName);
if (shapeNode) {
const indices = shapeNode[1][INPUT_KEY] || [];
Expand Down
9 changes: 6 additions & 3 deletions src/chart_types/partition_chart/state/chart_state.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { isTooltipVisibleSelector } from './selectors/is_tooltip_visible';
import { createOnElementClickCaller } from './selectors/on_element_click_caller';
import { createOnElementOutCaller } from './selectors/on_element_out_caller';
import { createOnElementOverCaller } from './selectors/on_element_over_caller';
import { getPieSpec } from './selectors/pie_spec';
import { getPartitionSpec } from './selectors/partition_spec';
import { getTooltipInfoSelector } from './selectors/tooltip';

/** @internal */
Expand All @@ -55,7 +55,7 @@ export class PartitionState implements InternalChartState {
}

isInitialized(globalState: GlobalChartState) {
return getPieSpec(globalState) !== null ? InitStatus.Initialized : InitStatus.SpecNotInitialized;
return getPartitionSpec(globalState) !== null ? InitStatus.Initialized : InitStatus.SpecNotInitialized;
}

isBrushAvailable() {
Expand Down Expand Up @@ -99,7 +99,10 @@ export class PartitionState implements InternalChartState {
}

isTooltipVisible(globalState: GlobalChartState) {
return { visible: isTooltipVisibleSelector(globalState), isExternal: false };
return {
visible: isTooltipVisibleSelector(globalState),
isExternal: false,
};
}

getTooltipInfo(globalState: GlobalChartState) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ import { isHierarchicalLegend } from '../../../../utils/legend';
import { QuadViewModel } from '../../layout/types/viewmodel_types';
import { map } from '../iterables';
import { partitionGeometries } from './geometries';
import { getPieSpec } from './pie_spec';
import { getPartitionSpec } from './partition_spec';

/** @internal */
export const computeLegendSelector = createCachedSelector(
[getPieSpec, getSettingsSpecSelector, partitionGeometries],
[getPartitionSpec, getSettingsSpecSelector, partitionGeometries],
(pieSpec, { flatLegend, legendMaxDepth, legendPosition }, { quadViewModel }): LegendItem[] => {
if (!pieSpec) {
return [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { HierarchyOfArrays, CHILDREN_KEY } from '../../layout/utils/group_by_rollup';
import { PartitionSpec } from '../../specs';
import { getPieSpec } from './pie_spec';
import { getPartitionSpec } from './partition_spec';
import { getTree } from './tree';

/** @internal */
export const getLegendItemsExtra = createCachedSelector(
[getPieSpec, getSettingsSpecSelector, getTree],
[getPartitionSpec, getSettingsSpecSelector, getTree],
(pieSpec, { legendMaxDepth }, tree): Map<SeriesKey, LegendItemExtraValues> => {
const legendExtraValues = new Map<SeriesKey, LegendItemExtraValues>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ import { LegendItemLabel } from '../../../../state/selectors/get_legend_items_la
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { CHILDREN_KEY, HierarchyOfArrays, HIERARCHY_ROOT_KEY } from '../../layout/utils/group_by_rollup';
import { Layer } from '../../specs';
import { getPieSpec } from './pie_spec';
import { getPartitionSpec } from './partition_spec';
import { getTree } from './tree';

/** @internal */
export const getLegendItemsLabels = createCachedSelector(
[getPieSpec, getSettingsSpecSelector, getTree],
[getPartitionSpec, getSettingsSpecSelector, getTree],
(pieSpec, { legendMaxDepth }, tree): LegendItemLabel[] =>
pieSpec ? flatSlicesNames(pieSpec.layers, 0, tree).filter(({ depth }) => depth <= legendMaxDepth) : [],
)(getChartIdSelector);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ import { GlobalChartState, PointerState } from '../../../../state/chart_state';
import { getLastClickSelector } from '../../../../state/selectors/get_last_click';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { isClicking } from '../../../../state/utils';
import { getPartitionSpec } from './partition_spec';
import { getPickedShapesLayerValues } from './picked_shapes';
import { getPieSpec } from './pie_spec';

/**
* Will call the onElementClick listener every time the following preconditions are met:
Expand All @@ -43,7 +43,7 @@ export function createOnElementClickCaller(): (state: GlobalChartState) => void
return (state: GlobalChartState) => {
if (selector === null && state.chartType === ChartTypes.Partition) {
selector = createCachedSelector(
[getPieSpec, getLastClickSelector, getSettingsSpecSelector, getPickedShapesLayerValues],
[getPartitionSpec, getLastClickSelector, getSettingsSpecSelector, getPickedShapesLayerValues],
(pieSpec, lastClick: PointerState | null, settings: SettingsSpec, pickedShapes): void => {
if (!pieSpec) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ import { ChartTypes } from '../../..';
import { GlobalChartState } from '../../../../state/chart_state';
import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { getPartitionSpec } from './partition_spec';
import { getPickedShapesLayerValues } from './picked_shapes';
import { getPieSpec } from './pie_spec';

/**
* Will call the onElementOut listener every time the following preconditions are met:
Expand All @@ -39,7 +39,7 @@ export function createOnElementOutCaller(): (state: GlobalChartState) => void {
return (state: GlobalChartState) => {
if (selector === null && state.chartType === ChartTypes.Partition) {
selector = createCachedSelector(
[getPieSpec, getPickedShapesLayerValues, getSettingsSpecSelector],
[getPartitionSpec, getPickedShapesLayerValues, getSettingsSpecSelector],
(pieSpec, pickedShapes, settings): void => {
if (!pieSpec) {
return;
Expand Down
Loading