feat(partition): monotonic font size scaling #681

monfera · 2020-05-25T13:17:51Z

Summary

Fixes Partition chart text should scale uniformly along sectors/rings #661 by implementing monotonic font size scaling
Turns the font size selection from O(N) to O(log(N)), resulting in one order of magnitude speedup for complex cases (many nodes, broad font size range, fine font size increments)
The hill climber, initially used for just linked label text truncation, is extracted and reused for this
The hill climber accuracy was improved, and the PR exploits this by truncating less of the linked label texts
Very large functions in fill_text_layout.ts got broken up, and some changes were needed by this (typed arg lists) as well as by switching to the hill climber, but otherwise its logic remained unchanged (fill part of mocks didn't change except the two changed 3-layer ones)
Improves typing in general, adds some generics and retires a few anys

Details

The feature implements per-layer monotonic font scaling.

The 3-layer sunburst and treemap have per layer knobs to see the effect.

The tradeoff is reduced scaling in legibility: a single bad case (eg. narrow box, long word) can yield a small, maybe unrenderably small text for all the subsequent boxes too, even if those could otherwise be rendered. For this reason, and also, not to cause a breaking change, the current PR has layers: [ { fillLabel: { monotonic: false } } ] as a default; can be changed if it's important enough to cause a breaking change. Also, maybe the current logic could be augmented by still allowing the use of the smallest font size even if there was a larger value with text that didn't fit even with the smallest font size.

The logic currently achieves monotonicity per layer. This is uncontroversial with the treemap as groove texts are bound to be small. For sunburst, it looks preferable too, because otherwise a single, small inner slice can lead to small fonts in all the outer rings. However it can still lead to cases where an outer sector has a larger font than an inner sector of lower value, eg. Mineral fuels vs Machinery / North America (can be the other way around too). Once we introduce global monotonicity, new tradeoffs of diminishing returns arise 😸

The per layer monotonicity also means that font size constraints do not get reset for groups. In other words, if there are two top level groups A and B in a two-layer chart, then the font size of a node in Group A can't be larger than any of the font sizes of nodes in Group B that have higher values.

There's no constraint for linearity or other proportionality. So it's still possible to have unbalanced texts, eg. if the biggest box is "China" and a very close runner-up is "United States" then the font size for the latter will be unproportionately small, also depending on box aspect ratios.

The current issue and PR response leaves other aspects of "text ink weight perception" unaddressed: text lengths and even the character shapes in the texts impact visual weight. Further, possible improvements might help, but again, there are diminishing returns, so prioritization would be needed:

shortening texts with ...
line break with hyphenation for long fill text words
letting the chart choose from an alternative set of text lengths as eg. SAP does, eg.
- "UK" (or ISO "GB" / "GBR")
- "United Kingdom"
- "United Kingdom of Great Britain and Northern Ireland"
shoot for a horizontal aspect ratio for nodes with long words (general problem might be NP-hard)

These would bring improvements in addition to helping with text ink proportionality

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

codecov-commenter · 2020-05-25T13:24:01Z

Codecov Report

Merging #681 into master will increase coverage by 0.30%.
The diff coverage is 93.82%.

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
+ Coverage   72.63%   72.94%   +0.30%     
==========================================
  Files         266      266              
  Lines        8686     8711      +25     
  Branches     1696     1696              
==========================================
+ Hits         6309     6354      +45     
+ Misses       2338     2318      -20     
  Partials       39       39

Impacted Files	Coverage Δ
...hart_types/partition_chart/layout/config/config.ts	`68.75% <ø> (ø)`
...types/partition_chart/layout/types/config_types.ts	`100.00% <ø> (ø)`
...es/partition_chart/layout/types/viewmodel_types.ts	`80.00% <ø> (ø)`
...rtition_chart/layout/viewmodel/link_text_layout.ts	`15.51% <16.66%> (+5.39%)`	⬆️
.../chart_types/partition_chart/layout/utils/calcs.ts	`78.57% <92.10%> (+28.57%)`	⬆️
...rtition_chart/layout/viewmodel/fill_text_layout.ts	`96.01% <98.03%> (+0.70%)`	⬆️
..._types/partition_chart/layout/circline_geometry.ts	`98.43% <100.00%> (ø)`
...ypes/partition_chart/layout/viewmodel/viewmodel.ts	`89.14% <100.00%> (-0.09%)`	⬇️
...n_chart/state/selectors/get_legend_items_labels.ts	`94.59% <100.00%> (ø)`

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 9429dcf...0759cdd. Read the comment docs.

markov00 · 2020-05-25T14:02:45Z

src/chart_types/partition_chart/layout/config/config.ts

@@ -151,6 +151,10 @@ export const configMetadata = {
    type: 'number',
    reconfigurable: false, // there's no real reason to reconfigure it; finding the largest possible font is good for readability
  },
+  monotonic: {
+    dflt: false,


I don't have a strong opinion, but to me seems that a monotonic font scale avoids misunderstanding when reading the chart. I prefer having it on by default. The consumer then can opt to turn it off to maximize the font size depending on space. As you already highlighted in the PR description it's a tradeoff between readability and understandability.
@rshen91 @nickofthyme what do you think?
We should not be afraid of turning it on and mark the PR as breaking change, but we should prefer giving the user a good default.

Glad to switch, I was on the fence about it and the config is there either way. A single long word or narrow box can turn subsequent boxes less legible, if at all (see PR desc.) ie. assumes more about the data, iow more vulnerable input-wise. But, monotonically shrinking text is preattentively preferable 😄 The breaking change tipped the balance for me.

...maybe it's not even a breaking change? We don't have an API contract that specifies text maximization. We should retain freedom for changing data and text ink that honors API contracts. Worth a discussion, a strict interpretation would require a breaking change for any pixel level visual change, while a too loose interpretation would throw curveballs to dependency maintainers

Yeah I agree, should default to true. I don't know if making it a breaking change is necessary though, It's not that noticeable of a change in most cases and it always looks better, IMHO.

I don't have strong feelings, but I think it's great we're making this available. I'm leaning towards the stance stated above, it's probably best to set it on by default and just have the option for consumers to set it off if readability suffers. I don't think it's a breaking change and it's nice that the consumer can always set it to false if they liked the font sizing beforehand.

Thanks for the common thinking, done in e166746

markov00 · 2020-05-25T14:04:43Z

src/chart_types/partition_chart/layout/types/config_types.ts

@@ -62,6 +62,7 @@ export interface FillFontSizeRange {
  minFontSize: Pixels;
  maxFontSize: Pixels;
  idealFontSizeJump: Ratio;
+  monotonic: boolean;


Add a tsdoc to describe the effect of this property monotonic

Suggested change

monotonic: boolean;

/** the fonts are scaled in.... **/

monotonic: boolean;

note: it got renamed in a subsequent commit e166746

markov00 · 2020-05-25T14:05:28Z

src/chart_types/partition_chart/layout/utils/calcs.ts

+type NumberMap = (n: number) => number;
+
+/** @internal */
+export function monotonicHillClimb(


even if this method is internal, a quick description of this method in a tsdoc is useful

markov00 · 2020-05-25T14:08:55Z

src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts

@@ -182,7 +182,7 @@ function rectangleConstruction(treeHeight: number, topGroove: number) {
 }

 /** @internal */
-export function shapeViewModel(
+export function shapeViewModel<C>(


I don't see where we are using this generic, do we really need it?

Thanks, good catch, it was used at some point but no more, will remove

src/chart_types/partition_chart/layout/viewmodel/link_text_layout.ts

src/chart_types/partition_chart/layout/types/geometry_types.ts

src/chart_types/partition_chart/layout/utils/calcs.test.ts

src/chart_types/partition_chart/layout/utils/calcs.ts

nickofthyme · 2020-05-26T14:39:25Z

src/chart_types/partition_chart/layout/viewmodel/fill_text_layout.ts

+        fontWeight,
+        valueFormatter,
+        padding,
+      } = Object.assign(


can you add a type here that this merge is going to resolve to?

Suggested change

} = Object.assign(

}: MyType = Object.assign(

There's currently no specific type for this, and making one up for this wouldn't be terribly informative as it's just the reiteration of what's in the const bracket. I'm planning further extraction from this function (not in this PR though) as it's still too large, in which case a proper boundary, carrying these resolved/cascaded values, needs to arise. But it needs a bit of design work, glad to chat about it on a call separately or on the team sync, the upshot is that I feel uncomfortable typing this transitional thing without that design. Still open to being convinced if I can learn about the reasons 👍

Sure that's fine, this merging of several values is just hard to decipher. Even if the types were defined inline that would be better, something like...

Suggested change

} = Object.assign(

}: {

fontWeight: number,

valueFormatter: () => string,

padding: number,

} = Object.assign(

Or even a combination of types that satisfy the resolved values, surely you have all the types to combine into the correct values.

Suggested change

} = Object.assign(

}: MyFontType & Pick<MyStyleType, 'style'> = Object.assign(

Anyway, this in fine for now just wanted to start the discussion. 👍

src/chart_types/partition_chart/layout/viewmodel/fill_text_layout.ts

…-squashed # Conflicts: # integration/tests/__image_snapshots__/all-test-ts-baseline-visual-tests-for-all-stories-sunburst-sunburst-with-two-layers-visually-looks-correct-1-snap.png

# [19.4.0](v19.3.0...v19.4.0) (2020-05-28) ### Bug Fixes * **partition:** consider legendMaxDepth on legend size ([#654](#654)) ([9429dcf](9429dcf)), closes [#639](#639) ### Features * **partition:** enable grooves in all group layers ([#666](#666)) ([f5b4767](f5b4767)) * **partition:** linked text overflow avoidance ([#670](#670)) ([b6e5911](b6e5911)), closes [#633](#633) * **partition:** monotonic font size scaling ([#681](#681)) ([ea2489b](ea2489b)), closes [#661](#661) * **tooltip:** improve positioning with popperjs ([#651](#651)) ([6512950](6512950)), closes [#596](#596)

markov00 · 2020-05-28T14:32:16Z

🎉 This PR is included in version 19.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

monfera added 2 commits May 25, 2020 14:19

feat: globally monotonous fill text sizes

941db80

refactor: text size calc improvements

b7697b7

monfera added enhancement New feature or request chore :partition Partition/PieChart/Donut/Sunburst/Treemap chart related labels May 25, 2020

monfera self-assigned this May 25, 2020

monfera requested a review from markov00 May 25, 2020 13:17

chore: extracted API

d453fa0

markov00 reviewed May 25, 2020

View reviewed changes

nickofthyme reviewed May 26, 2020

View reviewed changes

monfera and others added 12 commits May 26, 2020 16:43

chore: name simplification

dde2d84

feat: activate monotonicity

d217024

chore: comment

0629ff2

chore: monotonicHillClimb doc

2537397

fix: remove useless generic arg

9372a7e

chore: added return type

853f1fa

feat: monotonic to maximizeFontSize

5e9a3a0

test: partition mocks flipping to monotonic

e166746

chore: charts api extraction

3d940f1

Merge remote-tracking branch 'origin/master' into monotonic-text-size…

4c28328

…-squashed # Conflicts: # integration/tests/__image_snapshots__/all-test-ts-baseline-visual-tests-for-all-stories-sunburst-sunburst-with-two-layers-visually-looks-correct-1-snap.png

test: master merge image update update

b5805e4

fix: api changes

0759cdd

nickofthyme approved these changes May 27, 2020

View reviewed changes

monfera merged commit ea2489b into elastic:master May 27, 2020

markov00 added the released Issue released publicly label May 28, 2020

markov00 mentioned this pull request Jun 15, 2020

Update dependency @elastic/charts to v19.5.2 elastic/kibana#69126

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(partition): monotonic font size scaling #681

feat(partition): monotonic font size scaling #681

monfera commented May 25, 2020

codecov-commenter commented May 25, 2020 •

edited

Loading

markov00 May 25, 2020

monfera May 25, 2020

monfera May 26, 2020

nickofthyme May 26, 2020 •

edited

Loading

rshen91 May 26, 2020

monfera May 26, 2020

markov00 May 25, 2020

monfera May 26, 2020

monfera May 26, 2020

markov00 May 25, 2020

monfera May 26, 2020

markov00 May 25, 2020

monfera May 25, 2020

monfera May 26, 2020

nickofthyme May 26, 2020

monfera May 26, 2020

nickofthyme May 27, 2020

nickofthyme May 27, 2020

nickofthyme May 27, 2020

markov00 commented May 28, 2020

	monotonic: boolean;
	/ the fonts are scaled in.... /
	monotonic: boolean;

-      } = Object.assign(
+      }: {
+        fontWeight: number,
+        valueFormatter: () => string,
+        padding: number,
+      } = Object.assign(

	} = Object.assign(
	}: MyFontType & Pick<MyStyleType, 'style'> = Object.assign(

feat(partition): monotonic font size scaling #681

feat(partition): monotonic font size scaling #681

Conversation

monfera commented May 25, 2020

Summary

Details

Checklist

codecov-commenter commented May 25, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickofthyme May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 commented May 28, 2020

codecov-commenter commented May 25, 2020 •

edited

Loading

nickofthyme May 26, 2020 •

edited

Loading