feat(partition): treemap group text in grooves

[monfera]

Summary

• Puts treemap text for groups into top grooves
• Treemap texts are left-aligned and brought to top (groove texts are on the bottom)
• Font min/max sizes can be configured per Layer

To limit areal distortion, the groove heights are capped by a proportion of the area.

Closes #611
Fixes the treemap part of #599
Ticks a few boxes in #612

Further iteration ideas:

• expose constraints via configurability, eg. maximal area distortion
• adherence to text padding - now it exploits maximum area

Checklist

Delete any items that are not applicable to this PR.

• 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, including a check against IE11
• 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

[markov00]

Code wise LGTM. Tested locally on storybook and it works great.
I've some minor thing to notice:

• the groove text is slightly horizontally misaligned if compared with its first rectangle text, I think we should use the same padding as the the sublayer

- as already specified on the comments, I think we should provide at least one simple way to opt-out from the groovetext, exposing the `topGroove` parameter is the simplest way to do this, or a simple boolean (on/off) is fine on the config.

[nickofthyme]

Looks great Robert! Love the new options.

I only have two comments about the code.

[nickofthyme]

This is fine for this PR, but nested ternarys are screaming to be a separate function. Makes it so much cleaner and easier to read. 😱

[monfera]

Oops, meant to break it up, thanks for spotting this

[monfera]

I extracted it out into a function with switch / case

1 file changed, 31 insertions(+), 7 deletions(-)

and while there are best practices that morph into rules with broad community approval, eg. don't nest ternaries, I'm not sure if it makes the code much more readable, at least I personally don't mind short, chained ternaries, they're easier for me to read than esp.

let rowCentroidY = null; 
if(....) {
  rowCentroidY = ....
} else if { 
.
.
.

because here there's no guarantee that something else further down doesn't modify the let binding.

[monfera]

done in 6d3acea

[nickofthyme]

This kind of combined type is confusing. I think these should be separate types. This implies that the value VerticalAlignments has the same types as the VerticalAlignments which is not the case.

Fro example VerticalAlignments object does not have 'alphabetic' as a possible value.

Suggested change

	export type VerticalAlignment = CanvasTextBaseline & $Values<typeof VerticalAlignments>;
	export type VerticalAlignment = $Values<typeof VerticalAlignments>;
	export type AllVerticalAlignment = CanvasTextBaseline & VerticalAlignment;

[monfera]

It can certainly be broken up eg.

type PermittedVerticalAlignments = $Values<typeof VerticalAlignments>;
export type VerticalAlignment = CanvasTextBaseline & PermittedVerticalAlignments;

which just lifts and names the $Values part. Would that be clearer code? Your suggestion is the same extraction but I don't follow the namings as AllVerticalAlignments seems to hint that it's a broader / union set, while the purpose of the & is to intersect the two sets. Also, both do not need to be exported, but maybe you have some non-local change in mind.

OK why the heck did I do this in the first place, ie. why not just the expected $Values<typeof VerticalAlignments>? The &-ing will not in fact remove values from $Values<typeof VerticalAlignments> (one is a proper subset of the other). The purpose is to show intent and assurance to the compiler and code reader that

• values are expected to be directly digestible by the Canvas2 API without compile time type juggling or worse, runtime type guards, as it's directly channeled into a ctx.textBaseline so it better be of CanvasTextBaseline(*)
• yet, as you say, we don't currently expose all the alignments

There is a larger, somewhat theme and it'd be great to talk about it on account of this, we touched on esp. text styling configurability superficially with @markov00 and it'd be opportune to use this as a small example.

[monfera]

broken up, as mentioned, in a slightly different way in cb141a6

[nickofthyme]

Yeah the AllVerticalAlignments was just an arbitrary name I cam up with.

The change in cb141a6 is close to what I was referring to but the key is that the object with the js values and the type should be called the same, that way ts can import the Type and Values as one import.

// also using this
type SupportedVerticalAlignments = $Values<typeof VerticalAlignments>;

// makes this look strange
const myThing: SupportedVerticalAlignments = VerticalAlignments.top

These values are basically the poor mans enum because tsc enums are not ideally transpiled (see elastic/kibana#62957 (comment)), with this thinking, your enum values in this case would be different than the enum type.

To me the below is the ideal way to defined such enums.

export const VerticalAlignments = Object.freeze({
  top: 'top' as 'top',
  middle: 'middle' as 'middle',	
  bottom: 'bottom' as 'bottom',
});
type VerticalAlignments = $Values<typeof VerticalAlignments>;

// then it becomes much clearer how the type and enum values relate
const myThing: VerticalAlignments = VerticalAlignments.top

Could be something to talk about today at the sync 🤷

[monfera]

Yes, the enum would have various weaknesses here, one of which here is that it's not some opaque thing I want here, but a subtype of CanvasTextBaseline which is a very real set of strings.

If I understand, your remaining ask is that the object and type be named identically; it's doable, though with a bit less directness here as there's the above mentioned goal of piggybacking on the CanvasTextBaseline type, in 9c30090

[monfera]

@markov00 on the horizontal misalignment between group and leaf labels: there's still a padding at the group level, but it's leftward a bit, because the group wraps all the leafs, and to cause space between groups, it's larger (btw. the large groove on the top is just an increase of this grooves, but the left, right and bottom grooves aren't zeros either).

It'd be good to further improve on the alignment but it could be a small, follow-up prioritized item as we can do multiple things here - currently I'm leaning toward changing the groove sizing such that the left group groove is zero and the right group groove is double of what it is now, but it needs a bit of t(h)in(er)king. An alternative is to add an extra constraint on the groove text box but that's some additional code path. Just offsetting the text would be a third option, but it'd feel like a hack (long text might protrude on the right) and the vertical alignment would be accidental.

[nickofthyme]

I made a note on the enum comment with one final tweak if you could fix that before merging. Otherwise, everything looks great!

[markov00]

🎉 This PR is included in version 19.1.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀