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.

Sign up for GitHub

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

test(xy): scale type improvements #1381

Merged
merged 30 commits into from
Sep 16, 2021
Merged

test(xy): scale type improvements #1381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
543c33c
chore: tick values are numbers
monfera Sep 16, 2021
915c86f
chore: showing any via the generic type
monfera Sep 16, 2021
9ed546a
chore: converting more any to unknown
monfera Sep 16, 2021
12ef5a8
chore: converting more any to unknown 02
monfera Sep 16, 2021
c7f89b2
chore: converting more any to unknown 03
monfera Sep 16, 2021
26d9935
chore: converting more any to unknown 04
monfera Sep 16, 2021
8675664
chore: converting more any to unknown 05
monfera Sep 16, 2021
2ae39b3
chore: converting more any to real type 06
monfera Sep 16, 2021
57c5c8f
chore: converting more any to real type 07
monfera Sep 16, 2021
3607f0a
chore: converting more any to real type 08
monfera Sep 16, 2021
ae17f5c
chore: converting more any to real type 09
monfera Sep 16, 2021
bb93b5a
chore: converting more any to real type 10
monfera Sep 16, 2021
432238d
chore: converting more any to real type 11
monfera Sep 16, 2021
9be0b6a
chore: converting more any to real type 12
monfera Sep 16, 2021
770f3a6
chore: generic ScaleBand 01
monfera Sep 16, 2021
44af026
chore: generic ScaleBand 02
monfera Sep 16, 2021
6eea0f7
chore: generic ScaleBand 03
monfera Sep 16, 2021
0ec33ef
chore: converting more any to real type 13
monfera Sep 16, 2021
7272653
chore: converting more any to real type 14
monfera Sep 16, 2021
5bc5c0a
chore: converting more any to real type 15
monfera Sep 16, 2021
3e30380
chore: converting more any to real type 16
monfera Sep 16, 2021
09bed4a
refactor: drive by removal of verbose code
monfera Sep 16, 2021
c20cef9
chore: converting more any to real type 17
monfera Sep 16, 2021
0ac3fd2
test: type improvement round ht MarcoV NickP
monfera Sep 16, 2021
8cd6cda
test: generic typing for isBandScale
monfera Sep 16, 2021
afaceb6
test: doing away with the unknown
monfera Sep 16, 2021
b59e2e0
test: more assertion removals due to review update ripple-down
monfera Sep 16, 2021
b36aefb
test: more any removals
monfera Sep 16, 2021
640d644
test: all yScale occurrences are numeric
monfera Sep 16, 2021
36d1596
test: minor other unifications
monfera Sep 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/charts/src/chart_types/xy_chart/annotations/line/dimensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { AnnotationLineProps } from './types';

function computeYDomainLineAnnotationDimensions(
annotationSpec: LineAnnotationSpec,
yScale: Scale,
yScale: Scale<number>,
{ vertical, horizontal }: SmallMultipleScales,
chartRotation: Rotation,
axisPosition?: Position,
Expand Down Expand Up @@ -118,7 +118,7 @@ function computeYDomainLineAnnotationDimensions(

function computeXDomainLineAnnotationDimensions(
annotationSpec: LineAnnotationSpec,
xScale: Scale,
xScale: Scale<number | string>,
{ vertical, horizontal }: SmallMultipleScales,
chartRotation: Rotation,
isHistogramMode: boolean,
Expand Down Expand Up @@ -233,8 +233,8 @@ function computeXDomainLineAnnotationDimensions(
export function computeLineAnnotationDimensions(
annotationSpec: LineAnnotationSpec,
chartRotation: Rotation,
yScales: Map<GroupId, Scale>,
xScale: Scale,
yScales: Map<GroupId, Scale<number>>,
xScale: Scale<number | string>,
smallMultipleScales: SmallMultipleScales,
isHistogramMode: boolean,
axisPosition?: Position,
Expand Down
8 changes: 4 additions & 4 deletions packages/charts/src/chart_types/xy_chart/annotations/rect/dimensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ export function isWithinRectBounds({ x, y }: Point, { startX, endX, startY, endY
/** @internal */
export function computeRectAnnotationDimensions(
annotationSpec: RectAnnotationSpec,
yScales: Map<GroupId, Scale>,
xScale: Scale,
yScales: Map<GroupId, Scale<number>>,
xScale: Scale<number>,
axesSpecs: AxisSpec[],
smallMultiplesScales: SmallMultipleScales,
chartRotation: Rotation,
Expand Down Expand Up @@ -162,7 +162,7 @@ export function computeRectAnnotationDimensions(
}

function scaleXonBandScale(
xScale: ScaleBand,
xScale: ScaleBand<number | string>,
x0: PrimitiveValue,
x1: PrimitiveValue,
): { x: number; width: number } | null {
Expand Down Expand Up @@ -223,7 +223,7 @@ function scaleXonContinuousScale(
* @param isHistogram
*/
function limitValueToDomainRange(
scale: Scale,
scale: Scale<number>,
minValue?: PrimitiveValue,
maxValue?: PrimitiveValue,
isHistogram = false,
Expand Down
34 changes: 17 additions & 17 deletions packages/charts/src/chart_types/xy_chart/annotations/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,8 @@ export function invertTransformedCursor(
export function computeAnnotationDimensions(
annotations: AnnotationSpec[],
chartRotation: Rotation,
yScales: Map<GroupId, Scale>,
xScale: Scale,
yScales: Map<GroupId, Scale<number>>,
xScale: Scale<number | string>,
axesSpecs: AxisSpec[],
isHistogramModeEnabled: boolean,
smallMultipleScales: SmallMultipleScales,
Expand All @@ -145,22 +145,22 @@ export function computeAnnotationDimensions(
annotationDimensions.set(id, dimensions);
}
return annotationDimensions;
}

const dimensions = computeRectAnnotationDimensions(
annotationSpec,
yScales,
xScale,
axesSpecs,
smallMultipleScales,
chartRotation,
getAxisStyle,
isHistogramModeEnabled,
);
} else {
const dimensions = computeRectAnnotationDimensions(
annotationSpec,
yScales,
xScale as Scale<number>,
Copy link
Member

Choose a reason for hiding this comment

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

why you are restricting this to number only if the passed one could be both?

Copy link
Member

Choose a reason for hiding this comment

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

You can change computeRectAnnotationDimensions to accept Scale<unknown> because internally you are calling isBandScale that assert the type for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re the question, it's because it's in a conditional branch, and I didn't want to add potentially unduly restrictive types in the other, unrelated branch in the first round of improvements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't change computeRectAnnotationDimensions to accept Scale<unknown> because calling limitValueToDomainRange will have a red squiggly mark (limitValueToDomainRange performs a subtraction of domain values, so it must be numeric) and for the same reason it can't be Scale<number | string> either

Basically the computeRectAnnotationDimensions only seems to work with numeric X, though I haven't tested the runtime, it was enough for me to see the subtraction. Though the subtraction only happens if the domain length is non-zero, I assumed it's always the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By extension, the entire computeRectAnnotationDimensions only works with a numeric X scale, Marco or Nick lmk if it's not the case

axesSpecs,
smallMultipleScales,
chartRotation,
getAxisStyle,
isHistogramModeEnabled,
);

if (dimensions) {
annotationDimensions.set(id, dimensions);
if (dimensions) {
annotationDimensions.set(id, dimensions);
}
return annotationDimensions;
}
return annotationDimensions;
}, new Map());
}
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/xy_chart/crosshair/crosshair_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const DEFAULT_SNAP_POSITION_BAND = 1;
/** @internal */
export function getSnapPosition(
value: string | number,
scale: Scale,
scale: Scale<number | string>,
totalBarsInCluster = 1,
): { band: number; position: number } | undefined {
const position = scale.scale(value);
Expand Down Expand Up @@ -86,7 +86,7 @@ export function getCursorBandPosition(
withinBandwidth: boolean;
},
snapEnabled: boolean,
xScale: Scale,
xScale: Scale<number | string>,
totalBarsInCluster?: number,
): Rect | undefined {
const { top, left, width, height } = panel;
Expand Down
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/xy_chart/rendering/area.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ import {
export function renderArea(
shift: number,
dataSeries: DataSeries,
xScale: Scale,
yScale: Scale,
xScale: Scale<number | string>,
yScale: Scale<number>,
panel: Dimensions,
color: Color,
curve: CurveType,
Expand Down
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/xy_chart/rendering/bars.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ type BarTuple = {
export function renderBars(
orderIndex: number,
dataSeries: DataSeries,
xScale: Scale,
yScale: Scale,
xScale: Scale<number | string>,
yScale: Scale<number>,
panel: Dimensions,
chartRotation: number,
minBarHeight: number,
Expand Down
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/xy_chart/rendering/bubble.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import { MarkSizeOptions } from './utils';
export function renderBubble(
shift: number,
dataSeries: DataSeries,
xScale: Scale,
yScale: Scale,
xScale: Scale<number | string>,
yScale: Scale<number>,
color: Color,
panel: Dimensions,
hasY0Accessors: boolean,
Expand Down
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/xy_chart/rendering/line.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ import {
export function renderLine(
shift: number,
dataSeries: DataSeries,
xScale: Scale,
yScale: Scale,
xScale: Scale<number | string>,
yScale: Scale<number>,
panel: Dimensions,
color: Color,
curve: CurveType,
Expand Down
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/xy_chart/rendering/points.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ import {
export function renderPoints(
shift: number,
dataSeries: DataSeries,
xScale: Scale,
yScale: Scale,
xScale: Scale<number | string>,
yScale: Scale<number>,
panel: Dimensions,
color: Color,
pointStyle: PointStyle,
Expand Down
15 changes: 10 additions & 5 deletions packages/charts/src/chart_types/xy_chart/rendering/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ export function isDatumFilled({ filled, initialY1 }: DataSeriesDatum) {
* @param xScaleOffset
* @internal
*/
export function getClippedRanges(dataset: DataSeriesDatum[], xScale: Scale, xScaleOffset: number): ClippedRanges {
export function getClippedRanges(
dataset: DataSeriesDatum[],
xScale: Scale<number | string>,
xScaleOffset: number,
): ClippedRanges {
let firstNonNullX: number | null = null;
let hasNull = false;

Expand Down Expand Up @@ -153,7 +157,7 @@ export function isPointOnGeometry(
return yCoordinate >= y && yCoordinate <= y + height && xCoordinate >= x && xCoordinate <= x + width;
}

const getScaleTypeValueValidator = (yScale: Scale): ((n: number) => boolean) => {
const getScaleTypeValueValidator = (yScale: Scale<number>): ((n: number) => boolean) => {
if (!isLogarithmicScale(yScale)) return () => true;

const domainPolarity = getDomainPolarity(yScale.domain);
Expand All @@ -174,7 +178,7 @@ export type YDefinedFn = (
) => boolean;

/** @internal */
export function isYValueDefinedFn(yScale: Scale, xScale: Scale): YDefinedFn {
export function isYValueDefinedFn(yScale: Scale<number>, xScale: Scale<number | string>): YDefinedFn {
const validator = getScaleTypeValueValidator(yScale);
return (datum, getValueAccessor) => {
const yValue = getValueAccessor(datum);
Expand All @@ -184,6 +188,7 @@ export function isYValueDefinedFn(yScale: Scale, xScale: Scale): YDefinedFn {

/** @internal */
export const CHROME_PINCH_BUG_EPSILON = 0.5;

/**
* Temporary fix for Chromium bug
* Shift a small pixel value when pixel diff is <= 0.5px
Expand All @@ -196,7 +201,7 @@ function chromeRenderBugBuffer(y1: number, y0: number): number {
}

/** @internal */
export function getY1ScaledValueOrThrowFn(yScale: Scale): (datum: DataSeriesDatum) => number {
export function getY1ScaledValueOrThrowFn(yScale: Scale<number>): (datum: DataSeriesDatum) => number {
const datumAccessor = getYDatumValueFn();
const scaleY0Value = getY0ScaledValueOrThrowFn(yScale);
return (datum) => {
Expand All @@ -207,7 +212,7 @@ export function getY1ScaledValueOrThrowFn(yScale: Scale): (datum: DataSeriesDatu
}

/** @internal */
export function getY0ScaledValueOrThrowFn(yScale: Scale): (datum: DataSeriesDatum) => number {
export function getY0ScaledValueOrThrowFn(yScale: Scale<number>): (datum: DataSeriesDatum) => number {
const isLogScale = isLogarithmicScale(yScale);
const domainPolarity = getDomainPolarity(yScale.domain);
const logBaseline = domainPolarity >= 0 ? Math.min(...yScale.domain) : Math.max(...yScale.domain);
Expand Down
5 changes: 3 additions & 2 deletions packages/charts/src/chart_types/xy_chart/state/selectors/compute_annotations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
* Side Public License, v 1.
*/

import { Scale } from '../../../../scales';
import { createCustomCachedSelector } from '../../../../state/create_selector';
import { getChartThemeSelector } from '../../../../state/selectors/get_chart_theme';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { AnnotationId, AxisId } from '../../../../utils/ids';
import { AnnotationId, AxisId, GroupId } from '../../../../utils/ids';
import { AnnotationDimensions } from '../../annotations/types';
import { computeAnnotationDimensions } from '../../annotations/utils';
import { computeSeriesGeometriesSelector } from './compute_series_geometries';
Expand Down Expand Up @@ -44,7 +45,7 @@ export const computeAnnotationDimensionsSelector = createCustomCachedSelector(
return computeAnnotationDimensions(
annotationSpecs,
settingsSpec.rotation,
yScales,
yScales as Map<GroupId, Scale<number>>,
xScale,
axesSpecs,
isHistogramMode,
Expand Down
12 changes: 8 additions & 4 deletions packages/charts/src/chart_types/xy_chart/state/selectors/compute_small_multiple_scales.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import { computeSeriesDomainsSelector } from './compute_series_domains';

/** @internal */
export interface SmallMultipleScales {
horizontal: ScaleBand;
vertical: ScaleBand;
horizontal: ScaleBand<number | string>;
vertical: ScaleBand<number | string>;
}

/**
Expand All @@ -43,6 +43,10 @@ export function getScale(
padding: RelativeBandsPadding = DEFAULT_SM_PANEL_PADDING,
) {
const singlePanelSmallMultiple = domain.length <= 1;
const defaultDomain = domain.length === 0 ? [undefined] : domain;
return new ScaleBand(defaultDomain, [0, maxRange], undefined, singlePanelSmallMultiple ? 0 : padding);
return new ScaleBand(
domain.length > 0 ? domain : [(undefined as unknown) as number],
Copy link
Contributor Author

@monfera monfera Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes we've already been cheating to comply with the D3 scale typedef file and the D3 doc, but now we're clear about it. I'll try to solve it in a next PR, it's only partially addressed in this one

[0, maxRange],
undefined,
singlePanelSmallMultiple ? 0 : padding,
);
}
2 changes: 1 addition & 1 deletion packages/charts/src/chart_types/xy_chart/state/selectors/get_cursor_band.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ function getCursorBand(
externalPointerEvent: PointerEvent | null,
chartDimensions: Dimensions,
settingsSpec: SettingsSpec,
xScale: Scale | undefined,
xScale: Scale<number | string> | undefined,
seriesSpecs: BasicSeriesSpec[],
totalBarsInCluster: number,
isTooltipSnapEnabled: boolean,
Expand Down
5 changes: 4 additions & 1 deletion packages/charts/src/chart_types/xy_chart/state/selectors/get_projected_pointer_position.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ function getProjectedPointerPosition(
};
}

function getPosRelativeToPanel(panelScale: ScaleBand, pos: number): { pos: number; value: PrimitiveValue } {
function getPosRelativeToPanel(
panelScale: ScaleBand<number | string>,
pos: number,
): { pos: number; value: PrimitiveValue } {
const outerPadding = panelScale.outerPadding * panelScale.step;
const innerPadding = panelScale.innerPadding * panelScale.step;
const numOfDomainSteps = panelScale.domain.length;
Expand Down
2 changes: 1 addition & 1 deletion packages/charts/src/chart_types/xy_chart/state/selectors/is_tooltip_snap_enabled.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@ export const isTooltipSnapEnableSelector = createCustomCachedSelector(
(seriesGeometries, snap) => isTooltipSnapEnabled(seriesGeometries.scales.xScale, snap),
);

function isTooltipSnapEnabled(xScale: Scale, snap: boolean) {
function isTooltipSnapEnabled(xScale: Scale<number | string>, snap: boolean) {
return (xScale && xScale.bandwidth > 0) || snap;
}
10 changes: 5 additions & 5 deletions packages/charts/src/chart_types/xy_chart/state/selectors/on_brush_end_caller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export function createOnBrushEndCaller(): (state: GlobalChartState) => void {
lastDrag,
rotation,
histogramMode,
xScale,
xScale as Scale<number>,
smallMultipleScales,
minBrushDelta,
roundHistogramBrushValues,
Expand Down Expand Up @@ -134,7 +134,7 @@ function getXBrushExtent(
lastDrag: DragState,
rotation: Rotation,
histogramMode: boolean,
xScale: Scale,
xScale: Scale<number>,
smallMultipleScales: SmallMultipleScales,
minBrushDelta?: number,
roundHistogramBrushValues?: boolean,
Expand Down Expand Up @@ -190,7 +190,7 @@ function getYBrushExtents(
chartDimensions: Dimensions,
lastDrag: DragState,
rotation: Rotation,
yScales: Map<GroupId, Scale>,
yScales: Map<GroupId, Scale<number | string>>,
smallMultipleScales: SmallMultipleScales,
minBrushDelta?: number,
): GroupBrushExtent[] | undefined {
Expand All @@ -212,8 +212,8 @@ function getYBrushExtents(

const minPosScaled = yScale.invert(minPos);
const maxPosScaled = yScale.invert(maxPos);
const minValue = clamp(minPosScaled, yScale.domain[0], maxPosScaled);
const maxValue = clamp(minPosScaled, maxPosScaled, yScale.domain[1]);
const minValue = clamp(minPosScaled, (yScale as Scale<number>).domain[0], maxPosScaled);
const maxValue = clamp(minPosScaled, maxPosScaled, (yScale as Scale<number>).domain[1]);
yValues.push({ extent: [minValue, maxValue], groupId });
});
return yValues.length === 0 ? undefined : yValues;
Expand Down
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/xy_chart/state/utils/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ export interface GeometriesCounts {

/** @internal */
export interface ComputedScales {
xScale: Scale;
yScales: Map<GroupId, Scale>;
xScale: Scale<number | string>;
yScales: Map<GroupId, Scale<number | string>>;
}

/** @internal */
Expand Down
Loading