feat(partition): legend hover options

[monfera]

Summary

Implements 6 strategies in place of the current single strategy:

• key (the current and default one): highlight all identically named items, no matter where they are
• keyInLayer: highlight all identically named items which are within the same depth (ring) as the hovered legend depth
• node: highlight exact match in the path only
• path: highlight members of the exact path; ie. exact match in the path, plus all its ancestors
• nodeWithDescendants: highlight exact match in the path, and everything that is its descendant in that branch
• pathWithDescendants (gif below): highlight exact match in the path, and everything that is its ancestor, or its descendant in that branch

Example use:

<Settings
        showLegend
        flatLegend
        legendStrategy="pathWithDescendants"
        legendMaxDepth={maxDepth}
        theme={STORYBOOK_LIGHT_THEME}
      />

Closes #982 in addition.

Checklist

• Improve on the API as needed
• Extract the new API and commit
• Fix the temporary way of getting legendStrategy from the global spec
• Add legendStrategy to another mock, eg. the three-layered sunburst as on the image above
• Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
• This was checked for cross-browser compatibility
• Proper documentation or storybook story was added for features that require explanation or tutorials
• Unit tests were updated or added to match the most common scenarios

[monfera]

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

[nickofthyme]

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

[monfera]

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)

[nickofthyme]

CC: @markov00 @rshen91

[monfera]

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

[codecov-io]

Codecov Report

Merging #978 (fa82675) into master (6c4dafd) will increase coverage by 0.05%.
The diff coverage is 65.00%.

@@            Coverage Diff             @@
##           master     #978      +/-   ##
==========================================
+ Coverage   70.87%   70.92%   +0.05%     
==========================================
  Files         344      361      +17     
  Lines       10971    10629     -342     
  Branches     2309     2064     -245     
==========================================
- Hits         7776     7539     -237     
+ Misses       3181     3002     -179     
- Partials       14       88      +74     

Flag	Coverage Δ
unittests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rt_types/heatmap/state/selectors/compute_legend.ts	42.85% <ø> (+2.85%)	⬆️
...es/partition_chart/layout/types/viewmodel_types.ts	83.33% <ø> (ø)
src/chart_types/xy_chart/legend/legend.ts	100.00% <ø> (ø)
src/specs/settings.tsx	86.66% <ø> (-0.44%)	⬇️
src/state/chart_state.ts	85.91% <ø> (-2.00%)	⬇️
...on_chart/state/selectors/get_highlighted_shapes.ts	33.33% <26.08%> (-22.23%)	⬇️
...es/heatmap/state/selectors/get_grid_full_height.ts	28.12% <50.00%> (+1.65%)	⬆️
...artition_chart/renderer/dom/highlighter_legend.tsx	44.44% <50.00%> (ø)
.../partition_chart/state/selectors/compute_legend.ts	78.37% <75.00%> (-2.18%)	⬇️
...es/partition_chart/layout/utils/group_by_rollup.ts	87.09% <100.00%> (ø)
... and 216 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c4dafd...fa82675. Read the comment docs.

[nickofthyme]

Love this so much, it is so pleasant on the eyes to change the strategy and see all the options.

I have a few questions, suggestions and comments.

[nickofthyme]

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? 🤔

[monfera]

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.

[monfera]

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.

[nickofthyme]

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.

[monfera]

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

[nickofthyme]

Can we make this an exported enum?

Suggested change

	/** @public */
	export type LegendStrategy = 'node' | 'path' | 'key' | 'keyInLayer' | 'nodeWithDescendants' | 'pathWithDescendants'; // keyof typeof legendStrategies
	export const LegendStrategy = Object.freeze({
	/** add description for each */
	Node: 'node' as const,
	Path: 'path' as const,
	Key: 'key' as const,
	KeyInLayer: 'keyInLayer' as const,
	NodeWithDescendants: 'nodeWithDescendants' as const,
	});
	/** @public */
	export type LegendStrategy = $Values<typeof LegendStrategy>;

[monfera]

Yes, possible, what are the pros & cons of that? I can start with the cons 😃

• the single line became 10 lines
• there's much repetition and broilerplate: every prop name occurs twice, and as const occurs everywhere
• we make the API more complex by needing to export this object, and the user needs to use it, for no benefit I can currently think of
• it's a nit, but it's not an enum (we're not using enum anywhere); it's an enum-inspired object representation

In short, why multiply related code size for both ourselves and userland code, while also reducing the DRYness? I may overlook some technical benefits.

Even as it is, I'm not happy with the repetition of keys of the legendStrategies object, as it's not DRY, but one repetition feels better than two 😃 Would be great if some TS code transform could change keyof typeof legendStrategies to the corresponding union type of literals for cases when legendStrategies is unexported because then we could just say,

export type LegendStrategy = keyof typeof legendStrategies;

without having to export `legendStrategies

[nickofthyme]

Using "enums" in this case is more explicit and structured than having to set the string values. I think all your cons are really pros 😅

• the single line became 10 lines -- Yes but this is a non-issue with minification
• there's much repetition and broilerplate: every prop name occurs twice, and as const occurs everywhere
  we make the API more complex by needing to export this object, and the user needs to use it, for no benefit I can currently think of -- Yes but this also allows us to communicate the intention of each of the enum properties to the end user with makes the API BETTER not more complex. Otherwise, the end user would have to go into the codebase src to see what the options do beyond the simple string property.
• it's a nit, but it's not an enum (we're not using enum anywhere); it's an enum-inspired object representation -- True but that's just because the enum support in babel is not yet sufficient.

What do you mean by repetition? The const enum exports a type and value under the same name making it easy to set it as a type or value.

[monfera]

this is a non-issue with minification

What do you mean by repetition?

My concern wasn't line count or code size in the bundle, though even there it's bigger; I just don't currently see the value of the "enum" object, compared to the union of string literals. Frankly it's tedious to type up the bigger one, given the choice I'd type less, for identical results 😅

Yes but this also allows us to communicate the intention of each of the enum properties to the end user with makes the API BETTER

Otherwise, the end user would have to go into the codebase src to see what the options do beyond the simple string property

How does your alternative snippet with the { 'optionName': 'optionName' } communicate the intent of each option better than the current commit with 'optionName'? Looks like the same amount of information; and all of them use strings (as the "enum" object keys are strings).

The string version autocompletes for library users just like the "enum" version because it's a union of string literals.

But the "enum" version involves the export of LegendOptions as a runtime object, not just as a type. I don't feel that increasing the API surface with an extra object, where a string suffices, is desirable unless it buys something I don't see yet.

Or maybe it's a hypothetical scenario where the "enum" prop keys would change, or become much longer, like { 'Highlight Node With All Descendants': 'nodeWithDescendants' } ? Please give an example to support that the "enum" is better for the library user.

makes the API BETTER not more complex

Under what scenario is legendOption: 'path' more complex than legendOption: LegendOptions.path?

True but that's just because the enum support in babel is not yet sufficient

It may be for the better; I'm not super keen on adding code constructs that don't bring in a lot of value but bifurcate TS from JS. So it's not a critique for not using enum—avoidance on transpilation, polyfills etc. is generally a good thing.

It's a small change and I'm glad to switch to an object for library consistency (we use the "enum" pattern a lot), or if it has tangible benefits; I'm trying to learn what the benefits are, so a specific example of the value would be neat.

[markov00]

Few pros about the current "enum" way of exposing constants:

• consistency: we used the real ts enum when we started the library, but then we switched to the "enum" objects pattern later on due to missing transpilation from babel. I rather prefer to do that change all our codebase at the same time, then expose a few things in a way and others in a different one.
• type safety: we can easily change the value for those enums constants when needed without creating breaking changes for the user. if my enum has {Path: 'path'} but I need to change path to path_key I can do that without breaking changes.
• type safety 2: when used in JS environment, providing an enum object is a bit safer then let the user throw in unwanted strings
• discoverability: with a union type of strings and in a TS environment you can discover the possible values only within the method that uses that union type. Instead, when using an enum object you can easily declare where ever you need your constant and keep using your IDE to discover what other values are available.
• discoverability 2: with an enum object you can enumerate the available options and expose them as they are if needed. This probably is not a very good practice, but for automated testing or within our storybook we can iterate over enum object keys and produce selection knobs easily without the need to refactor them on change
• documentation: you can document each element of the enum object easily, where I have never seen that for a union type.

This is basically very similar to the concept of having generic keys for our objects like you have discussed few comments above with the use of [SOME_KEY]: value

And, btw, no one blocks the user from using directly the enum value instead of passing from the object e.g.:

export const PartitionLayout = Object.freeze({
  sunburst: 'sunburst' as const,
  treemap: 'treemap' as const,
  icicle: 'icicle' as const,
  flame
})
// I can use the "standard" way
<Partition config={{ partitionLayout: PartitionLayout.sunburst }} />
// or the simplified, no TS error will be thrown
<Partition config={{ partitionLayout: 'sunburst' }} />

[monfera]

Thanks folks for the comments so far, I think the consistency argument trumps other aspects–if it's a convention and we don't use strings elsewhere, it's best to either stick to it or revise in bulk, which would be a big change for debatable benefit–, and the discoverability 2 item is something I didn't think of. Let's go through the rest at some point.

[monfera]

8e6bd45

[nickofthyme]

Yeah I can see you points Robert on the added surface of the API but my thought about it being clearer to the user was not { 'optionName': 'optionName' } but rather the const enum allows for each key in the object to be defined clearly, much like the Fit enum below. In this case we can add a larger more descriptive explanation of each option. So rather than just the string carry we can say Use the previous non-nullvalue.

elastic-charts/src/chart_types/xy_chart/utils/specs.ts

Lines 174 to 256 in 6c4dafd

	export const Fit = Object.freeze({
	/**
	* Don't draw value on the graph. Slices out area between `null` values.
	*
	* @example
	* ```js
	* [2, null, null, 8] => [2, null null, 8]
	* ```
	*/
	None: 'none' as const,
	/**
	* Use the previous non-`null` value
	*
	* @remarks
	* This is the opposite of `Fit.Lookahead`
	*
	* @example
	* ```js
	* [2, null, null, 8] => [2, 2, 2, 8]
	* ```
	*/
	Carry: 'carry' as const,
	/**
	* Use the next non-`null` value
	*
	* @remarks
	* This is the opposite of `Fit.Carry`
	*
	* @example
	* ```js
	* [2, null, null, 8] => [2, 8, 8, 8]
	* ```
	*/
	Lookahead: 'lookahead' as const,
	/**
	* Use the closest non-`null` value (before or after)
	*
	* @example
	* ```js
	* [2, null, null, 8] => [2, 2, 8, 8]
	* ```
	*/
	Nearest: 'nearest' as const,
	/**
	* Average between the closest non-`null` values
	*
	* @example
	* ```js
	* [2, null, null, 8] => [2, 5, 5, 8]
	* ```
	*/
	Average: 'average' as const,
	/**
	* Linear interpolation between the closest non-`null` values
	*
	* @example
	* ```js
	* [2, null, null, 8] => [2, 4, 6, 8]
	* ```
	*/
	Linear: 'linear' as const,
	/**
	* Sets all `null` values to `0`
	*
	* @example
	* ```js
	* [2, null, null, 8] => [2, 0, 0, 8]
	* ```
	*/
	Zero: 'zero' as const,
	/**
	* Specify an explicit value `X`
	*
	* @example
	* ```js
	* [2, null, null, 8] => [2, X, X, 8]
	* ```
	*/
	Explicit: 'explicit' as const,
	});
	/** @public */
	export type Fit = $Values<typeof Fit>;

Regarding the use of one or the other -- that is the beauty of the const enum, is that it is available when needed and always replaceable by a string value. The const enum can also make the consumer code nicer when looping through all available options for a chart configuration option.

Lastly, these const enums act much like a typescript enum, and once support is updated, we can replace all of them to use the real enum and have a much better end-user experience.

[nickofthyme]

If this is true why not just combine them into one?

[monfera]

Because they may diverge, and their uses are different. One way of viewing things is from the pov of TS as it is: structural typing, which may be a bit of a transitional duck tape. The alternative pov is, preparation for nominal typing which is way more meaningful.

For example, currently, we can add an angle in radians with an angle in degrees, b/c both are number and most of TS is structural (there's branding, classes etc. though).

[nickofthyme]

If that is the case why not just keep it as is?

[monfera]

Preparation for nominal typing; better documentation of the intent, and meaning of things.

And forced by this PR, because using words like series, seriesKey etc. may translate well to legend items in case of most Cartesians, but it would've been misleading (though TS-wise, structurally passing) for partition and maybe other chart types. In case of partitioning charts, each slice/rectangle may get its own legend item, while there's no concept of series (technically, it's a singleton series).

Leaving it named seriesKey here and legendKey here, or calling both strings would of course have worked, because current TypeScript only cares about a few aspects of the structure (hey it's even okay with extra properties except in literals, so it's not even structurally tight). It's great if someone, versed in dataviz and TS, but not privvy of implementation detail, can get a good sense of what the library is about, just by reading its types.

It'd also be great to identify core, cross-cutting aspects of the library, to unify things along robust concepts. Our discussions, slides etc. and the domain of dataviz has a bunch of central concepts eg. categories, which are invariant, ie. totally not implementation detail. It's great to lean on solid concepts in the library too, and link it to meaningful terms of the art, like we do with words like axis, scale etc. Meanwhile, the concept of series is related, but not quite identical.

Would be useful to chat about the topic of elevating such a concept at the type level on one of our regular meetings.

[monfera]

Oh btw. I might have misunderstood you; it'd be great to put CategoryKey or category related (or inherent dataviz related) words/types elsewhere in the code too, but this PR avoids venturing too much without further discussion. Yet it starts the discussion and common thinking around this, so I didn't shy away from making an initial, localized step that doesn't lock us in, yet share my slant

[nickofthyme]

Yeah I was just referring to the coupling of the types here and if that was needed. I think we should discuss how/when we want to get away from the use of series from an API perspective. Fine with me for now 👍🏼

[markov00]

Everything is fine for me, I've left just a few minor comments, that can easily be left as they are

[markov00]

nit: Since this is a constant value that is used here and exported we can use all uppercase for the name for consistency with the rest of the code

Suggested change

	export const hierarchyRootKey: Key = '__root_key__';
	export const HIERARCHY_ROOT_KEY: Key = '__root_key__';

[monfera]

fa82675

[markov00]

you can remove this comment as you actually renamed the function

[monfera]

oops, will do 😄

[monfera]

fa82675

[nickofthyme]

LGTM, thanks for the changes.

[markov00]

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀