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

feat(partition): legend hover options #978

Merged
merged 35 commits into from
Jan 19, 2021
Merged

feat(partition): legend hover options #978

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
4016fb0
chore: pure rename
monfera Dec 17, 2020
9d5c4ba
feat: hierarchical legend options
monfera Jan 11, 2021
8bc5182
test: update cartesian legend tests with identity value
monfera Jan 11, 2021
bdf84e9
test: update partition legend tests with path content
monfera Jan 11, 2021
a3a0842
feat: meaningful strategy names instead of numbers
monfera Jan 11, 2021
527baf5
chore: post rebase cleanup 1
monfera Jan 14, 2021
616e70e
chore: post rebase cleanup 2
monfera Jan 14, 2021
5becba9
chore: post rebase cleanup 3
monfera Jan 14, 2021
752cbfb
fix: clear highlight on out event action
monfera Jan 14, 2021
90f3900
fix: highlight should take the proper key for cartesians
monfera Jan 14, 2021
d5e2645
refactor: dry up notion of horizontal legend
monfera Jan 14, 2021
9a19947
test: update cartesians with test cases
monfera Jan 14, 2021
d4e592f
feat: config and story for legendStrategy
monfera Jan 14, 2021
9b75150
fix: better specs access ht Marco
monfera Jan 15, 2021
bdb6cb3
refactor: type update
monfera Jan 15, 2021
d49528b
refactor: type update 2
monfera Jan 15, 2021
47bfe57
test: legendstrategy knob for legend slash piechart
monfera Jan 15, 2021
8418941
Merge branch 'master' into partition-legend-hover-options
monfera Jan 15, 2021
f50ffb5
fix: corrections
monfera Jan 18, 2021
b5be2e1
chore: revert api changes
monfera Jan 18, 2021
6262d2e
chore: update api changes again to address gh diff issue
monfera Jan 18, 2021
1f36925
Merge remote-tracking branch 'origin/master' into partition-legend-ho…
monfera Jan 18, 2021
beebcea
Merge remote-tracking branch 'monfera/partition-legend-hover-options'…
monfera Jan 18, 2021
fb25f8f
chore: removed null as it became redundant ht Nick
monfera Jan 18, 2021
ebbd65f
fix: use a more specific special root key ht Nick
monfera Jan 18, 2021
b5acaf3
fix: ignore arbitrary root key bound by hierarchyRootKey
monfera Jan 18, 2021
5d69d93
chore: deactivate line break enforcer
monfera Jan 18, 2021
6a6ae39
Revert "chore: deactivate line break enforcer"
monfera Jan 18, 2021
0300e6a
fix: prefix the preexisting rule so that the rule actually works
monfera Jan 18, 2021
8e6bd45
refactor: move config string to pseudo enum object
monfera Jan 19, 2021
43c57b7
Merge branch 'master' into partition-legend-hover-options
monfera Jan 19, 2021
a53dd28
chore: remove unneeded freeze ht Nick
monfera Jan 19, 2021
5ec5535
refactor: extract out type ht Nick
monfera Jan 19, 2021
35400a1
Merge branch 'partition-legend-hover-options' of https://github.com/m…
monfera Jan 19, 2021
fa82675
fix: comment and constant
monfera Jan 19, 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
3,944 changes: 1,974 additions & 1,970 deletions api/charts.api.md
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/chart_types/heatmap/state/selectors/compute_legend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export const computeLegendSelector = createCachedSelector(
seriesIdentifier,
isSeriesHidden: deselectedDataSeries.some((dataSeries) => dataSeries.key === seriesIdentifier.key),
isToggleable: true,
path: [{ index: 0, value: seriesIdentifier.key }],
};
});
},
Expand Down
4 changes: 2 additions & 2 deletions src/chart_types/heatmap/state/selectors/get_grid_full_height.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { GlobalChartState } from '../../../../state/chart_state';
import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';
import { getLegendSizeSelector } from '../../../../state/selectors/get_legend_size';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { Position } from '../../../../utils/commons';
import { isHorizontalLegend } from '../../../../utils/legend';
import { Config } from '../../layout/types/config_types';
import { getHeatmapConfigSelector } from './get_heatmap_config';
import { getHeatmapTableSelector } from './get_heatmap_table';
Expand Down Expand Up @@ -54,7 +54,7 @@ export const getGridHeightParamsSelector = createCachedSelector(
const xAxisHeight = visible ? fontSize : 0;
const totalVerticalPadding = padding * 2;
let legendHeight = 0;
if (showLegend && (legendPosition === Position.Top || legendPosition === Position.Bottom)) {
if (showLegend && isHorizontalLegend(legendPosition)) {
markov00 marked this conversation as resolved.
Show resolved Hide resolved
legendHeight = maxLegendHeight ?? legendSize.height;
}
const verticalRemainingSpace = containerHeight - xAxisHeight - totalVerticalPadding - legendHeight;
Expand Down
6 changes: 4 additions & 2 deletions src/chart_types/partition_chart/layout/types/viewmodel_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
* under the License.
*/

import { CategoryKey } from '../../../../commons/category';
import { LegendPath } from '../../../../state/actions/legend';
import { Color } from '../../../../utils/commons';
import { config, ValueGetterName } from '../config/config';
import { ArrayNode, HierarchyOfArrays } from '../utils/group_by_rollup';
Expand Down Expand Up @@ -148,13 +150,13 @@ interface SectorGeomSpecY {
y1px: Distance;
}

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

export interface ShapeTreeNode extends TreeNode, SectorGeomSpecY {
yMidPx: Distance;
depth: number;
sortIndex: number;
path: number[];
path: LegendPath;
monfera marked this conversation as resolved.
Show resolved Hide resolved
dataName: DataName;
value: number;
parent: ArrayNode;
Expand Down
25 changes: 14 additions & 11 deletions 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 @@ -17,6 +17,8 @@
* under the License.
*/

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

Expand Down Expand Up @@ -46,7 +48,7 @@ export interface ArrayNode extends NodeDescriptor {
[CHILDREN_KEY]: HierarchyOfArrays;
[PARENT_KEY]: ArrayNode;
[SORT_INDEX_KEY]: number;
[PATH_KEY]: number[];
[PATH_KEY]: LegendPath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've noticed this a lot lately in your PRs so I might as well ask here.

Specifically around the [SOME_KEY]: value thing...

Was this a remnant of javascript? I wonder what the utility is for having the keys extracted like this if they are not shared across different types.

I think typescript will protect against bad keys and this seems overly verbose for no real gain.

@markov00 Thoughts? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can replace all occurrences of these with the values here, with no change whatsoever to runtime or static type check, and the motive isn't that it's remnant of JavaScript, iirc the code was TS off the bat anyway.

export const AGGREGATE_KEY = 'value';
export const STATISTICS_KEY = 'statistics';
export const DEPTH_KEY = 'depth';
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';

The benefit is the following. Certain generic data processing functions, eg. groupByRollup, are domain independent, ie. they're not semantically coupled to even data visualization. We might as well import such functions from a 3rd party utility, or we might consider extracting some generic data processing and other utilities into a separate package; even if we never want to do this, it's good practice to not lock in accidental things such as property names.

So, not locking in particular field names is just a way of avoiding tight coupling between the data processing functions and their uses. Now it'd be fairly easy to use whatever field names.

It doesn't currently yield some kind of monetary or other short term benefit, though we could move such very generic functions into the collection of relational operators, so I think it's OK as it's not costly to keep it around this way. Btw. tree processing in partition charts may be the only such area that has this kind of keying.

Still, we can discuss removing them, probably for its own PR if it falls that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about something else for the pro side: if we use literal property names eg. thing.path, then the key name occurrence is spread over the code base, and it's hard to find all occurrences.

  • grepping isn't good, because, if the property name is common, like with these, it'll find all kinds of additional hits; we could offset it with too long and too specific property names, but then it's even more typing, and name obfuscation, and is still error prone
  • TS helps, if all the uses are properly typed, and some type alias is used, but it's not trustworthy (due to structural typing; one number is just like another number) and can be too many indirections even when it works

In contrast, with the square bracket notation, you use your favorite shortcut to jump to the definition of the key eg. export const PATH_KEY... and you use another shortcut, and you get a list of all places where that property is used in the codebase, and nothing more. It's a bit like with functions: you can always jump to the function definition, and to the places where you use the function, unambiguously.

In theory.

Currently, we don't gain a lot of this benefit in groupByRollup where it has been around since inception, and there's occasional direct use eg. thing.parent etc. so it's watered up. It could be easily ruled out by using symbols, or symbol.for, but it's not worth it for groupByRollup.

In sum, it has some benefits, and minor runtime costs too, and it can go either way for groupByRollup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the thoughtful explanation.

I see some benefit but imagine doing this for all interfaces across the codebase for better searchability. That would be ridiculous. I think there could be a balance where it's beneficial.

As far as grepping issue, I think if we all be more strict on typing everything, this issue would go away and the code would be better for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there could be a balance where it's beneficial.

I agree. In current master, the main (or only?) place is the data transforms in group_by_rollup.ts and that has been that way it was merged over a year ago, so its use isn't growing afaik.

As far as grepping issue, I think if we all be more strict on typing everything, this issue would go away and the code would be better for it

The issue with relying on current TypeScript for this is that TS typing is structural (not really, but close enough for this point), and it's always possible for some places to redundantly define something, as long as the prop exists, and is the same name and value type, TS would have no complaint. With the move to nominal typing, we'll be able to have assurance.

So, it'd look unusual if most of our code were full of square brackets for property names, but the technique may be fine for the few generic data processing utilities we have and that we might add to. We might want to deliberate it together and maybe even remove the current square brackets too.

imagine doing this for all interfaces across the codebase for better searchability. That would be ridiculous.

Currently, most of our code, even where logic is quite general, is tightly bound to how we use it, in which case it'd be pointless indeed.

However, successful dataviz and data processing libraries such as D3 are doing something similar: you can specify accessor functions, as a way to tell your D3 function, which object property to reach for, as it may be some arbitrary dataset. Bostock wrote on it here

The named object properties a much more compact, lighter weight, less opinionated way of doing the same thing, eg. it doesn't rely on the module pattern ie. closures with getters/setters that underpins D3; it also doesn't rely on heavier weight constructs eg. class, or passing property names explicitly for each function.

So, in short, I believe that:

  • when writing generic data manipulation functions, ideally it works with arbitrary property names, even if we have one instance of naming scheme in the code
  • this might be one of the most lighthanded ways of achieving that
  • the cost isn't significant for the low number of such functions we have
  • a tangible benefit, ie. not just a notion of generality, is the xref / lookup ability, which isn't assured yet by TS

}

type HierarchyOfMaps = Map<Key, MapNode>;
Expand All @@ -55,11 +57,12 @@ interface MapNode extends NodeDescriptor {
[PARENT_KEY]?: ArrayNode;
}

export type PrimitiveValue = string | number | null; // there could be more but sufficient for now
type Key = PrimitiveValue;
/** @internal */
export const hierarchyRootKey: Key = '';
Copy link
Contributor Author

@monfera monfera Jan 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This replaces multiple uses of null that needed a bit of tribal knowledge, with no central binding

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this could just be something like __root_key__ or something. Just a thought.

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, it could be that too, Marco or Rachel, any preference? I think our runtime should rule out empty series labels or empty pie slice labels, as it'd lead to weird results for the user (unlabeled things, eg. a solitary unexplained color in the legend)

Copy link
Collaborator

Choose a reason for hiding this comment

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

CC: @markov00 @rshen91

Copy link
Contributor Author

@monfera monfera Jan 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Followed through on this suggestion, as we don't currently rule out the empty string when inputting (though we ignore them, see 2nd commit). Commits: ebbd65f and ebbd65f


export type PrimitiveValue = string | number | null; // there could be more but sufficient for now
type Key = CategoryKey;
export type Sorter = (a: number, b: number) => number;

type NodeSorter = (a: ArrayEntry, b: ArrayEntry) => number;

export const entryKey = ([key]: ArrayEntry) => key;
Expand Down Expand Up @@ -126,8 +129,8 @@ export function groupByRollup(
});
return p;
}, new Map());
if (reductionMap.get(null) !== void 0) {
statistics.globalAggregate = (reductionMap.get(null) as MapNode)[AGGREGATE_KEY];
if (reductionMap.get(hierarchyRootKey) !== undefined) {
statistics.globalAggregate = (reductionMap.get(hierarchyRootKey) as MapNode)[AGGREGATE_KEY];
}
return reductionMap;
}
Expand All @@ -139,11 +142,11 @@ function getRootArrayNode(): ArrayNode {
[DEPTH_KEY]: NaN,
[CHILDREN_KEY]: children,
[INPUT_KEY]: [] as number[],
[PATH_KEY]: [] as number[],
[PATH_KEY]: [] as LegendPath,
[SORT_INDEX_KEY]: 0,
[STATISTICS_KEY]: { globalAggregate: 0 },
};
return { ...bootstrap, [PARENT_KEY]: bootstrap } as ArrayNode; // TS doesn't yet handle bootstrapping but the `Omit` above retains guarantee for all props except `[PARENT_KEY`
return { ...bootstrap, [PARENT_KEY]: bootstrap } as ArrayNode; // TS doesn't yet handle bootstrapping but the `Omit` above retains guarantee for all props except `[PARENT_KEY]`
}

/** @internal */
Expand Down Expand Up @@ -180,9 +183,9 @@ export function mapsToArrays(root: HierarchyOfMaps, sorter: NodeSorter | null):
});
}; // with the current algo, decreasing order is important
const tree = groupByMap(root, getRootArrayNode());
const buildPaths = ([, mapNode]: ArrayEntry, currentPath: number[]) => {
const newPath = [...currentPath, mapNode[SORT_INDEX_KEY]];
mapNode[PATH_KEY] = newPath;
const buildPaths = ([key, mapNode]: ArrayEntry, currentPath: LegendPath) => {
const newPath = [...currentPath, { index: mapNode[SORT_INDEX_KEY], value: key }];
monfera marked this conversation as resolved.
Show resolved Hide resolved
mapNode[PATH_KEY] = newPath; // in-place mutation, so disabled `no-param-reassign`
mapNode.children.forEach((entry) => buildPaths(entry, newPath));
};
buildPaths(tree[0], []);
Expand Down
105 changes: 92 additions & 13 deletions 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,6 +23,7 @@ import { 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 { hierarchyRootKey } 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 @@ -96,71 +97,104 @@ describe('Retain hierarchy even with arbitrary names', () => {
{
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'A',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 0, value: 'A' },
],
depth: 0,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
},
{
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'A',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 0, value: 'A' },
{ index: 0, value: 'A' },
],
depth: 1,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
},
{
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'B',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 0, value: 'A' },
{ index: 1, value: 'B' },
],
depth: 1,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
},
{
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'B',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 1, value: 'B' },
],
depth: 0,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
},
{
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'A',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 1, value: 'B' },
{ index: 0, value: 'A' },
],
depth: 1,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
},
{
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'B',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 1, value: 'B' },
{ index: 1, value: 'B' },
],
depth: 1,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
},
{
childId: 'C',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'C',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 2, value: 'C' },
],
depth: 0,
label: 'C',
seriesIdentifier: { key: 'C', specId: 'spec1' },
},
{
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'A',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 2, value: 'C' },
{ index: 0, value: 'A' },
],
depth: 1,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
},
{
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'B',
path: [
{ index: 0, value: hierarchyRootKey },
{ index: 2, value: 'C' },
{ index: 1, value: 'B' },
],
depth: 1,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
Expand All @@ -174,15 +208,38 @@ describe('Retain hierarchy even with arbitrary names', () => {
{
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'A',
path: [
{
index: 0,
value: hierarchyRootKey,
},
{
index: 0,
value: 'A',
},
],
depth: 0,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
},
{
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'A',
path: [
{
index: 0,
value: hierarchyRootKey,
},
{
index: 0,
value: 'A',
},
{
index: 0,
value: 'A',
},
],

depth: 1,
label: 'A',
seriesIdentifier: { key: 'A', specId: 'spec1' },
Expand All @@ -196,15 +253,37 @@ describe('Retain hierarchy even with arbitrary names', () => {
{
childId: 'C',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'C',
path: [
{
index: 0,
value: hierarchyRootKey,
},
{
index: 0,
value: 'C',
},
],
depth: 0,
label: 'C',
seriesIdentifier: { key: 'C', specId: 'spec1' },
},
{
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
dataName: 'B',
path: [
{
index: 0,
value: hierarchyRootKey,
},
{
index: 0,
value: 'C',
},
{
index: 0,
value: 'B',
},
],
depth: 1,
label: 'B',
seriesIdentifier: { key: 'B', specId: 'spec1' },
Expand Down
4 changes: 2 additions & 2 deletions src/chart_types/partition_chart/renderer/dom/highlighter_legend.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,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 { partitionGeometries } from '../../state/selectors/geometries';
import { getHighlightedSectorsSelector } from '../../state/selectors/get_highlighted_shapes';
import { legendHoverHighlightNodes } from '../../state/selectors/get_highlighted_shapes';
import { HighlighterComponent, HighlighterProps, DEFAULT_PROPS } from './highlighter';

const legendMapStateToProps = (state: GlobalChartState): HighlighterProps => {
Expand All @@ -38,7 +38,7 @@ const legendMapStateToProps = (state: GlobalChartState): HighlighterProps => {
config: { partitionLayout },
} = partitionGeometries(state);

const geometries = getHighlightedSectorsSelector(state);
const geometries = legendHoverHighlightNodes(state);
const canvasDimension = getChartContainerDimensionsSelector(state);
return {
chartId,
Expand Down
Loading