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

feat(partition): monotonic font size scaling #681

Merged
merged 15 commits into from
May 27, 2020
Merged
4 changes: 2 additions & 2 deletions api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1518,8 +1518,8 @@ export interface XYChartSeriesIdentifier extends SeriesIdentifier {

// Warnings were encountered during analysis:
//
// src/chart_types/partition_chart/layout/types/config_types.ts:113:5 - (ae-forgotten-export) The symbol "TimeMs" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/layout/types/config_types.ts:114:5 - (ae-forgotten-export) The symbol "AnimKeyframe" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/layout/types/config_types.ts:117:5 - (ae-forgotten-export) The symbol "TimeMs" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/layout/types/config_types.ts:118:5 - (ae-forgotten-export) The symbol "AnimKeyframe" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/specs/index.ts:47:13 - (ae-forgotten-export) The symbol "NodeColorAccessor" needs to be exported by the entry point index.d.ts
// src/commons/series_id.ts:37:3 - (ae-forgotten-export) The symbol "SeriesKey" needs to be exported by the entry point index.d.ts

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 9 additions & 2 deletions src/chart_types/partition_chart/layout/circline_geometry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,14 @@
* specific language governing permissions and limitations
* under the License. */

import { CirclineArc, Circline, CirclinePredicate, Distance, PointObject, RingSector } from './types/geometry_types';
import {
CirclineArc,
Circline,
CirclinePredicate,
Distance,
PointObject,
RingSectorConstruction,
} from './types/geometry_types';
import { TAU } from './utils/math';

function euclideanDistance({ x: x1, y: y1 }: PointObject, { x: x2, y: y2 }: PointObject): Distance {
Expand Down Expand Up @@ -113,7 +120,7 @@ function circlineValidSectors(refC: CirclinePredicate, c: CirclineArc): Circline
}

/** @internal */
export function conjunctiveConstraint(constraints: RingSector, c: CirclineArc): CirclineArc[] {
export function conjunctiveConstraint(constraints: RingSectorConstruction, c: CirclineArc): CirclineArc[] {
// imperative, slightly optimized buildup of `valids` as it's in the hot loop:
let valids = [c];
for (let i = 0; i < constraints.length; i++) {
Expand Down
4 changes: 4 additions & 0 deletions src/chart_types/partition_chart/layout/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
maximizeFontSize: {
dflt: false,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...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

Copy link
Collaborator

@nickofthyme nickofthyme May 26, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the common thinking, done in e166746

type: 'boolean',
},
partitionLayout: {
dflt: PartitionLayout.sunburst,
type: 'string',
Expand Down
4 changes: 4 additions & 0 deletions src/chart_types/partition_chart/layout/types/config_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ export interface FillFontSizeRange {
minFontSize: Pixels;
maxFontSize: Pixels;
idealFontSizeJump: Ratio;
/** When `maximizeFontSize` is false (the default), text font will not be larger than font sizes in larger sectors/rectangles in the same pie chart,
* sunburst ring or treemap layer. When it is set to true, the largest font, not exceeding `maxFontSize`, that fits in the slice/sector/rectangle
* will be chosen for easier text readability, irrespective of the value. **/
maximizeFontSize: boolean;
}

// todo switch to `io-ts` style, generic way of combining static and runtime type info
Expand Down
18 changes: 9 additions & 9 deletions src/chart_types/partition_chart/layout/types/geometry_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,40 +31,40 @@ export type Radius = Cartesian;
export type Radian = Cartesian; // we measure angle in radians, and there's unity between radians and cartesian distances which is the whole point of radians; this is also relevant as we use small-angle approximations
export type Distance = Cartesian;

/* @internal */
/** @internal */
monfera marked this conversation as resolved.
Show resolved Hide resolved
export interface PointObject {
x: Coordinate;
y: Coordinate;
}

/* @internal */
/** @internal */
export type PointTuple = [Coordinate, Coordinate];

/* @internal */
/** @internal */
export type PointTuples = [PointTuple, ...PointTuple[]]; // at least one point

/* @internal */
/** @internal */
export class Circline {
x: Coordinate = NaN;
y: Coordinate = NaN;
r: Radius = NaN;
}

/* @internal */
/** @internal */
export interface CirclinePredicate extends Circline {
inside: boolean;
}

/* @internal */
/** @internal */
export interface CirclineArc extends Circline {
from: Radian;
to: Radian;
}

/* @internal */
/** @internal */
type CirclinePredicateSet = CirclinePredicate[];

/* @internal */
export type RingSector = CirclinePredicateSet;
/** @internal */
export type RingSectorConstruction = CirclinePredicateSet;

export type TimeMs = number;
20 changes: 10 additions & 10 deletions src/chart_types/partition_chart/layout/types/viewmodel_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { ArrayNode, HierarchyOfArrays } from '../utils/group_by_rollup';
import { Color } from '../../../../utils/commons';
import { VerticalAlignments } from '../viewmodel/viewmodel';

/* @internal */
/** @internal */
export type LinkLabelVM = {
link: PointTuples;
translate: PointTuple;
Expand All @@ -38,7 +38,7 @@ export type LinkLabelVM = {
valueFontSpec: Font;
};

/* @internal */
/** @internal */
export interface RowBox extends Font {
text: string;
width: Distance;
Expand All @@ -51,19 +51,19 @@ interface RowAnchor {
rowAnchorY: Coordinate;
}

/* @internal */
/** @internal */
export interface RowSpace extends RowAnchor {
maximumRowLength: Distance;
}

/* @internal */
/** @internal */
export interface TextRow extends RowAnchor {
length: number;
maximumLength: number;
rowWords: Array<RowBox>;
}

/* @internal */
/** @internal */
export interface RowSet {
id: string;
rows: Array<TextRow>;
Expand All @@ -75,22 +75,22 @@ export interface RowSet {
container?: any;
}

/* @internal */
/** @internal */
export interface QuadViewModel extends ShapeTreeNode {
strokeWidth: number;
strokeStyle: string;
fillColor: string;
}

/* @internal */
/** @internal */
export interface OutsideLinksViewModel {
points: Array<PointTuple>;
}

/* @internal */
/** @internal */
export type PickFunction = (x: Pixels, y: Pixels) => Array<QuadViewModel>;

/* @internal */
/** @internal */
export type ShapeViewModel = {
config: Config;
quadViewModel: QuadViewModel[];
Expand All @@ -102,7 +102,7 @@ export type ShapeViewModel = {
outerRadius: number;
};

/* @internal */
/** @internal */
export const nullShapeViewModel = (specifiedConfig?: Config, diskCenter?: PointObject): ShapeViewModel => ({
config: specifiedConfig || config,
quadViewModel: [],
Expand Down
127 changes: 127 additions & 0 deletions src/chart_types/partition_chart/layout/utils/calcs.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License. */

import { integerSnap, monotonicHillClimb } from './calcs';

describe('monotonicHillClimb', () => {
const arbitraryNumber = 27;

describe('continuous functions', () => {
test('linear case', () => {
expect(monotonicHillClimb((n: number) => n, 100, arbitraryNumber)).toBeCloseTo(arbitraryNumber, 6);
monfera marked this conversation as resolved.
Show resolved Hide resolved
});

test('flat case should yield `maxVar`', () => {
expect(monotonicHillClimb(() => arbitraryNumber, 100, 50)).toBeCloseTo(100, 6);
});

test('nonlinear case', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n), Math.PI / 2, Math.sqrt(2) / 2)).toBeCloseTo(Math.PI / 4, 6);
});

test('non-compliant for even `minVar` should yield NaN', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n), Math.PI / 2, -1)).toBeNaN();
});

test('`loVar > hiVar` should yield NaN', () => {
expect(
monotonicHillClimb(
(n: number) => Math.sin(n),
1,
arbitraryNumber,
(n: number) => n,
2,
),
).toBeNaN();
});

test('compliant for `maxVar` should yield `maxVar`', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n), Math.PI / 2, 1)).toBeCloseTo(Math.PI / 2, 6);
});

test('`loVar === hiVar`, compliant', () => {
expect(
monotonicHillClimb(
(n: number) => Math.sin(n),
Math.PI / 2,
1,
(n: number) => n,
Math.PI / 2,
),
).toBe(Math.PI / 2);
});

test('`loVar === hiVar`, non-compliant', () => {
expect(
monotonicHillClimb(
(n: number) => Math.sin(n),
Math.PI / 2,
Math.sqrt(2) / 2,
(n: number) => n,
Math.PI / 2,
),
).toBeNaN();
});
});

describe('integral domain functions', () => {
test('linear case', () => {
expect(monotonicHillClimb((n: number) => n, 100, arbitraryNumber, integerSnap)).toBe(arbitraryNumber);
});

test('flat case should yield `maxVar`', () => {
expect(monotonicHillClimb(() => arbitraryNumber, 100, 50)).toBe(100);
});

test('nonlinear case', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n / 10), 15, Math.sqrt(2) / 2, integerSnap)).toBe(7);
});

test('non-compliant for even `minVar` should yield NaN', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n), Math.PI / 2, -1, integerSnap)).toBeNaN();
});

test('`loVar > hiVar` should yield NaN', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n), 1, arbitraryNumber, integerSnap, 2)).toBeNaN();
});

test('compliant for `maxVar` should yield `maxVar`', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n / 10), 15, 1, integerSnap)).toBe(15);
});

test('`loVar === hiVar`, compliant', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n / 10), 15, 1, integerSnap, 15)).toBe(15);
});

test('`loVar === hiVar`, non-compliant', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n / 10), 15, Math.sqrt(2) / 2, integerSnap, 15)).toBeNaN();
});

test('`loVar + 1 === hiVar`, latter is compliant', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n / 10), 15, 1, integerSnap, 14)).toBe(15);
});

test('`loVar + 1 === hiVar`, only former is compliant', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n / 10), 15, 0.99, integerSnap, 14)).toBe(14);
});

test('`loVar + 1 === hiVar`, non-compliant', () => {
expect(monotonicHillClimb((n: number) => Math.sin(n / 10), 15, Math.sqrt(2) / 2, integerSnap, 14)).toBeNaN();
});
});
});
75 changes: 75 additions & 0 deletions src/chart_types/partition_chart/layout/utils/calcs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,78 @@ export function colorIsDark(color: Color) {
const a = rgba.hasOwnProperty('opacity') ? opacity : 1;
return r * 0.299 + g * 0.587 + b * 0.114 < a * 150;
}

/** @internal */
export function getTextColor(shapeFillColor: Color, textColor: Color, textInvertible: boolean) {
const { r: tr, g: tg, b: tb, opacity: to } = stringToRGB(textColor);
const backgroundIsDark = colorIsDark(shapeFillColor);
const specifiedTextColorIsDark = colorIsDark(textColor);
const inverseForContrast = textInvertible && specifiedTextColorIsDark === backgroundIsDark;
return inverseForContrast
? to === undefined
? `rgb(${255 - tr}, ${255 - tg}, ${255 - tb})`
: `rgba(${255 - tr}, ${255 - tg}, ${255 - tb}, ${to})`
: textColor;
}

/** @internal */
export function integerSnap(n: number) {
return Math.floor(n);
}

type NumberMap = (n: number) => number;

/**
* `monotonicHillClimb` attempts to return a variable value that's associated with the highest valued response (as returned by invoking `getResponse`
* with said variable) yet still within the bounds for that response value, ie. constrained to smaller than or equal `responseUpperConstraint`.
* `minVar` and `maxVar` represent a closed interval constraint on the variable itself.
* `domainSnap` is useful if all real values in the range can't be assumed by the variable; typically, if the variable is integer only,
* such as the number of characters, or avoiding fractional font sizes.
* It is required that `getResponse` is a monotonic function over [minVar, maxVar], ie. a larger `n` value in this domain can't lead to
* a smaller return value. However, as it's an internal function with known use cases, there's no runtime check to assert this.
* Which is why the name expresses it prominently.
*/
/** @internal */
export function monotonicHillClimb(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getResponse: NumberMap,
maxVar: number,
responseUpperConstraint: number,
domainSnap: NumberMap = (n: number) => n,
minVar: number = 0,
) {
let loVar = domainSnap(minVar);
const loResponse = getResponse(loVar);
let hiVar = domainSnap(maxVar);
let hiResponse = getResponse(hiVar);

if (loResponse > responseUpperConstraint || loVar > hiVar) {
// bail if even the lowest value doesn't satisfy the constraint
return NaN;
}

if (hiResponse <= responseUpperConstraint) {
return hiVar; // early bail if maxVar is compliant
}

let pivotVar: number = NaN;
let pivotResponse: number = NaN;
let lastPivotResponse: number = NaN;
while (loVar < hiVar) {
const newPivotVar = (loVar + hiVar) / 2;
const newPivotResponse = getResponse(domainSnap(newPivotVar));
if (newPivotResponse === pivotResponse || newPivotResponse === lastPivotResponse) {
return domainSnap(loVar); // bail if we're good and not making further progress
}
pivotVar = newPivotVar;
lastPivotResponse = pivotResponse; // for prevention of bistable oscillation around discretization snap
pivotResponse = newPivotResponse;
const pivotIsCompliant = pivotResponse <= responseUpperConstraint;
if (pivotIsCompliant) {
loVar = pivotVar;
} else {
hiVar = pivotVar;
hiResponse = pivotResponse;
}
}
return domainSnap(pivotVar);
}
Loading