Skip to content

Commit a53a2ba

Browse files
monferamonfera
authored
refactor: scales (#1410)
Simplification and refactoring of Cartesian scale related code: - over 1k lines removed while leaving functionality intact - removed the undocumented `sortSeriesBy` which wasn't exercised by unit tests, VRT or Kibana, yet its former presence required multiple `@ts-ignore`s which are now also removed - removed disused code for animation - `Scale.scale()` and `Scale.pureScale()` always return a `number` instead of the union type (emitting`NaN` instead of `null` and downstream checks as before but with `Number.isNaN`, `Number.isFinite`, `scale(val) || 0` etc. so no behavioral change is expected) - solved four type assertions - replaced all the `isNaN`s with clearer alternatives (it coerces, eg. `isNaN(null)` and `isNaN("42")` yield `false`) - reworked some functions which were shallowly but verbosely wrapped in selectors - reworked `ScaleContinuous`, for example, the constuctor is split to tick count independent part vs tick count dependent part - combined `getAvailableTicks` and `getVisibleTicks` into a new `getVisibleTicks` (for this reason, unit tests that exercised these separately are currently commented out) - renamed some functions - removed syllogisms (where one of the ternary branches is a simple `true` or `false`, the ternary or `if/return` ladder is superfluous) - avoid object creation, code noise and line inflation via changing the signature of [`mergePartial`](https://github.com/elastic/elastic-charts/compare/31dab4a..7887bc8) - numerous other code simplifications (esp. in `axis_utils`), type updates (fewer optional parameters, fewer union types etc.) and minor fixes (eg. non-integer desired tick count) - discontinued `scaleOrThrow` and all other `...orThrow` functions, now just handling certain `NaN`/non-finiteness cases, except for geom string generation. These used to silently swallow errors in past (hypothetical? IRL experienced?) cases where some D3 scale were to return a non-finite value, or even, throw an error, so geoms silently don't show up. The new behavior is that if the D3 scale were to throw, there's an explicit, clear error, and if there's a `NaN` or similar, then the geom path will have it, and an explicit console log line will be written. There are pros&cons so this may be an item up for debate, as the nature and type of our defensiveness, and swallow vs. expose errors are concerned BREAKING CHANGE: LogScaleOptions.logBase` is now a `number` instead of the object enum `LogBase`. Some edge case data or configuration _might_, with a deemed low likelihood, lead to a situation where the earlier version would have silently not rendered a bar, line or point, while the new code doesn't `catch`, therefore throw an exception (see the last item). General risk of regressions due to the quantity of code changes (altogether 3.5k)
1 parent 5c55d65 commit a53a2ba

107 files changed

Lines changed: 1128 additions & 2305 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.eslintrc.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ module.exports = {
104104
'consistent-return': 0,
105105
'no-plusplus': 0,
106106
'no-bitwise': 0,
107-
'no-void': 0,
107+
'no-void': 1,
108108
yoda: 0,
109109
'no-restricted-globals': 0,
110110
'no-case-declarations': 0,

‎integration/tests/scales_stories.test.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
* Side Public License, v 1.
77
*/
88

9-
import { LogBase } from '../../packages/charts/src/scales/scale_continuous';
109
import { common } from '../page_objects';
1110

1211
describe('Scales stories', () => {
@@ -15,7 +14,7 @@ describe('Scales stories', () => {
1514
${'Negative'} | ${true}
1615
${'Positive'} | ${false}
1716
`('$polarity values', ({ value: negative }) => {
18-
it.each(Object.values(LogBase))('should render proper ticks with %s base', async (base) => {
17+
it.each(['common', 'binary', 'natural'])('should render proper ticks with %s base', async (base) => {
1918
await common.expectChartAtUrlToMatchScreenshot(
2019
`http://localhost:9001/?path=/story/scales--log-scale-options&knob-Use negative values_Y - Axis=${negative}&knob-Log base_Y - Axis=${base}&knob-Fit domain_Y - Axis=true&knob-Use default limit_Y - Axis=true`,
2120
);

‎packages/charts/api/charts.api.md‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1328,19 +1328,9 @@ export interface LineStyle {
13281328
visible: boolean;
13291329
}
13301330

1331-
// @public (undocumented)
1332-
export const LogBase: Readonly<{
1333-
Common: "common";
1334-
Binary: "binary";
1335-
Natural: "natural";
1336-
}>;
1337-
1338-
// @public
1339-
export type LogBase = $Values<typeof LogBase>;
1340-
13411331
// @public
13421332
export interface LogScaleOptions {
1343-
logBase?: LogBase;
1333+
logBase?: number;
13441334
logMinLimit?: number;
13451335
}
13461336

‎packages/charts/src/chart_types/goal_chart/state/selectors/scenegraph.ts‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,32 +26,23 @@ const mapConfigToTheme = ({
2626
fontFamily,
2727
}: RecursivePartial<Config> = {}): PartialTheme => ({
2828
chartMargins: margin,
29-
background: {
30-
color: backgroundColor,
31-
},
29+
background: { color: backgroundColor },
3230
goal: {
3331
minFontSize,
3432
maxFontSize,
35-
tickLabel: {
36-
fontFamily,
37-
},
38-
majorLabel: {
39-
fontFamily,
40-
},
41-
minorLabel: {
42-
fontFamily,
43-
},
33+
tickLabel: { fontFamily },
34+
majorLabel: { fontFamily },
35+
minorLabel: { fontFamily },
4436
},
4537
});
4638

4739
/** @internal */
4840
export function render(spec: GoalSpec, parentDimensions: Dimensions, theme: Theme): ShapeViewModel {
4941
// override theme and spec with old deprecated config options
50-
const mergedTheme: Theme = mergePartial(theme, mapConfigToTheme(spec.config), { mergeOptionalPartialValues: true });
42+
const mergedTheme: Theme = mergePartial(theme, mapConfigToTheme(spec.config));
5143
const mergedSpec: GoalSpec = mergePartial(spec, {
5244
angleEnd: spec?.config?.angleEnd,
5345
angleStart: spec?.config?.angleStart,
5446
});
55-
5647
return shapeViewModel(mergedSpec, mergedTheme, parentDimensions);
5748
}

‎packages/charts/src/chart_types/heatmap/layout/viewmodel/viewmodel.ts‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -56,18 +56,18 @@ function getValuesInRange(
5656
return values.slice(startIndex, endIndex);
5757
}
5858

59-
/**
60-
* Resolves the maximum number of ticks based on the chart width and sample label based on formatter config.
61-
*/
62-
function getTicks(chartWidth: number, { formatter, padding, fontSize, fontFamily }: Config['xAxisLabel']): number {
59+
function estimatedNonOverlappingTickCount(
60+
chartWidth: number,
61+
{ formatter, padding, fontSize, fontFamily }: Config['xAxisLabel'],
62+
): number {
6363
return withTextMeasure((textMeasure) => {
6464
const labelSample = formatter(Date.now());
6565
const { width } = textMeasure(labelSample, padding, fontSize, fontFamily);
66-
const maxTicks = Math.floor(chartWidth / width);
66+
const maxTicks = chartWidth / width;
6767
// Dividing by 2 is a temp fix to make sure {@link ScaleContinuous} won't produce
6868
// to many ticks creating nice rounded tick values
6969
// TODO add support for limiting the number of tick in {@link ScaleContinuous}
70-
return maxTicks / 2;
70+
return Math.floor(maxTicks / 2);
7171
});
7272
}
7373

@@ -89,13 +89,11 @@ export function shapeViewModel(
8989
const { table, yValues, xDomain } = heatmapTable;
9090

9191
// measure the text width of all rows values to get the grid area width
92-
const boxedYValues = yValues.map<Box & { value: NonNullable<PrimitiveValue> }>((value) => {
93-
return {
94-
text: config.yAxisLabel.formatter(value),
95-
value,
96-
...config.yAxisLabel,
97-
};
98-
});
92+
const boxedYValues = yValues.map<Box & { value: NonNullable<PrimitiveValue> }>((value) => ({
93+
text: config.yAxisLabel.formatter(value),
94+
value,
95+
...config.yAxisLabel,
96+
}));
9997

10098
// compute the scale for the rows positions
10199
const yScale = scaleBand<NonNullable<PrimitiveValue>>().domain(yValues).range([0, height]);
@@ -112,7 +110,7 @@ export function shapeViewModel(
112110
nice: false,
113111
},
114112
{
115-
desiredTickCount: getTicks(chartDimensions.width, config.xAxisLabel),
113+
desiredTickCount: estimatedNonOverlappingTickCount(chartDimensions.width, config.xAxisLabel),
116114
timeZone: config.timeZone,
117115
},
118116
)

‎packages/charts/src/chart_types/heatmap/state/selectors/get_heatmap_config.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,5 @@ import { getHeatmapSpecSelector } from './get_heatmap_spec';
1515
/** @internal */
1616
export const getHeatmapConfigSelector = createCustomCachedSelector(
1717
[getHeatmapSpecSelector],
18-
(spec): Config => {
19-
return mergePartial<Config>(defaultConfig, spec.config, { mergeOptionalPartialValues: true });
20-
},
18+
(spec): Config => mergePartial<Config>(defaultConfig, spec.config),
2119
);

‎packages/charts/src/chart_types/heatmap/state/selectors/get_x_axis_right_overflow.ts‎

Lines changed: 10 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,29 +19,15 @@ import { getHeatmapTableSelector } from './get_heatmap_table';
1919
*/
2020
export const getXAxisRightOverflow = createCustomCachedSelector(
2121
[getHeatmapConfigSelector, getHeatmapTableSelector],
22-
({ xAxisLabel: { fontSize, fontFamily, padding, formatter, width }, timeZone }, { xDomain }): number => {
23-
if (xDomain.type !== ScaleType.Time) {
24-
return 0;
25-
}
26-
if (typeof width === 'number') {
27-
return width / 2;
28-
}
29-
30-
const timeScale = new ScaleContinuous(
31-
{
32-
type: ScaleType.Time,
33-
domain: xDomain.domain as number[],
34-
range: [0, 1],
35-
},
36-
{
37-
timeZone,
38-
},
39-
);
40-
return withTextMeasure(
41-
(textMeasure) =>
42-
timeScale.ticks().reduce((acc, d) => {
43-
return Math.max(acc, textMeasure(formatter(d), padding, fontSize, fontFamily, 1).width + padding);
44-
}, 0) / 2,
45-
);
22+
({ xAxisLabel: { fontSize, fontFamily, padding, formatter, width }, timeZone }, { xDomain: { domain, type } }) => {
23+
return type !== ScaleType.Time
24+
? 0
25+
: typeof width === 'number'
26+
? width / 2
27+
: withTextMeasure((measure) =>
28+
new ScaleContinuous({ type: ScaleType.Time, domain: domain as number[], range: [0, 1] }, { timeZone })
29+
.ticks()
30+
.reduce((max, n) => Math.max(max, measure(formatter(n), padding, fontSize, fontFamily).width + padding), 0),
31+
) / 2;
4632
},
4733
);

‎packages/charts/src/chart_types/heatmap/state/selectors/scenegraph.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ export function render(
3939
const { width, height } = chartDimensions;
4040
const { config: specConfig } = spec;
4141
const partialConfig: RecursivePartial<Config> = { ...specConfig, width, height };
42-
const config = mergePartial<Config>(defaultConfig, partialConfig, { mergeOptionalPartialValues: true });
42+
const config = mergePartial<Config>(defaultConfig, partialConfig);
4343
return shapeViewModel(
4444
measureText(textMeasurerCtx),
4545
spec,

‎packages/charts/src/chart_types/partition_chart/layout/config.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ export type ValueGetterName = keyof typeof VALUE_GETTERS;
6666
function defaultFormatter(d: number): string {
6767
return Math.abs(d) >= 10000000 || Math.abs(d) < 0.001
6868
? d.toExponential(Math.min(2, Math.max(0, significantDigitCount(d) - 1)))
69-
: d.toLocaleString(void 0, {
69+
: d.toLocaleString(undefined, {
7070
maximumSignificantDigits: 4,
7171
maximumFractionDigits: 3,
7272
useGrouping: true,

‎packages/charts/src/chart_types/partition_chart/layout/viewmodel/fill_text_layout.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ function rowSetComplete(rowSet: RowSet, measuredBoxes: RowBox[]) {
151151
return (
152152
measuredBoxes.length === 0 &&
153153
!rowSet.rows.some(
154-
(r) => isNaN(r.length) || r.rowWords.length === 0 || r.rowWords.every((rw) => rw.text.length === 0),
154+
(r) => !Number.isFinite(r.length) || r.rowWords.length === 0 || r.rowWords.every((rw) => rw.text.length === 0),
155155
)
156156
);
157157
}
@@ -437,7 +437,7 @@ function getRowSet<C>(
437437
}
438438

439439
const { rowSet, completed } = tryFunction(identityRowSet(), fontSizes[fontSizeIndex]); // todo in the future, make the hill climber also yield the result to avoid this +1 call
440-
return { ...rowSet, rows: rowSet.rows.filter((r) => completed && !isNaN(r.length)) };
440+
return { ...rowSet, rows: rowSet.rows.filter((r) => completed && Number.isFinite(r.length)) };
441441
}
442442

443443
/** @internal */
@@ -512,12 +512,13 @@ export function fillTextLayout<C>(
512512
{ node, origin }: { node: QuadViewModel; origin: [Pixels, Pixels] },
513513
) => {
514514
const nextRowSet = filler(fontSizes, origin, node);
515+
const { fontSize } = nextRowSet;
515516
const layerIndex = node.depth - 1;
516517
return {
517518
rowSets: [...rowSets, nextRowSet],
518519
fontSizes: fontSizes.map((layerFontSizes: Pixels[], index: number) =>
519-
!isNaN(nextRowSet.fontSize) && index === layerIndex && !layers[layerIndex]?.fillLabel?.maximizeFontSize
520-
? layerFontSizes.filter((size: Pixels) => size <= nextRowSet.fontSize)
520+
Number.isFinite(fontSize) && index === layerIndex && !layers[layerIndex]?.fillLabel?.maximizeFontSize
521+
? layerFontSizes.filter((size: Pixels) => size <= fontSize)
521522
: layerFontSizes,
522523
),
523524
};

0 commit comments

Comments
 (0)