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

test(xy): scale type improvements #1381

Merged
merged 30 commits into from
Sep 16, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { Line } from '../../../../geoms/types';
import { Scale } from '../../../../scales';
import { Scale, ScaleBand } from '../../../../scales';
import { isContinuousScale, isBandScale } from '../../../../scales/types';
import { isNil, Position, Rotation } from '../../../../utils/common';
import { Dimensions, Size } from '../../../../utils/dimensions';
Expand Down Expand Up @@ -118,7 +118,7 @@ function computeYDomainLineAnnotationDimensions(

function computeXDomainLineAnnotationDimensions(
annotationSpec: LineAnnotationSpec,
xScale: Scale<unknown>,
xScale: Scale<string | number>,
{ vertical, horizontal }: SmallMultipleScales,
chartRotation: Rotation,
isHistogramMode: boolean,
Expand Down Expand Up @@ -164,10 +164,10 @@ function computeXDomainLineAnnotationDimensions(
}
} else if (isBandScale(xScale)) {
if (isHistogramMode) {
const padding = (xScale.step - xScale.originalBandwidth) / 2;
const padding = (xScale.step - (xScale as ScaleBand<string | number>).originalBandwidth) / 2;
annotationValueXPosition -= padding;
} else {
annotationValueXPosition += xScale.originalBandwidth / 2;
annotationValueXPosition += (xScale as ScaleBand<string | number>).originalBandwidth / 2;
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove this assertion because isBandScale already define xScale as ScaleBand

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, will check! I haven't added assertions where there was no complaint, but it's possible that a later commit made an earlier assertion unnecessary, indeed a good idea to revisit all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I only later turned the type guard function generic, now it no longer complains there 👯‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
} else {
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
return;
Expand Down Expand Up @@ -234,7 +234,7 @@ export function computeLineAnnotationDimensions(
annotationSpec: LineAnnotationSpec,
chartRotation: Rotation,
yScales: Map<GroupId, Scale<number>>,
xScale: Scale<unknown>,
xScale: Scale<string | number>,
smallMultipleScales: SmallMultipleScales,
isHistogramMode: boolean,
axisPosition?: Position,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function computeRectAnnotationDimensions(
let xAndWidth: { x: number; width: number } | null = null;

if (isBandScale(xScale)) {
xAndWidth = scaleXonBandScale(xScale, x0, x1);
xAndWidth = scaleXonBandScale(xScale as ScaleBand<number>, x0, x1);
Copy link
Member

Choose a reason for hiding this comment

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

this one also seems unnecessary, see my previous comment in file /xy_chart/annotations/utils.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for the catch, I need to revisit these (the assertion was done before the function param types were tightened)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
} else if (isContinuousScale(xScale)) {
xAndWidth = scaleXonContinuousScale(xScale, x0, x1, isHistogram);
}
Expand Down Expand Up @@ -162,7 +162,7 @@ export function computeRectAnnotationDimensions(
}

function scaleXonBandScale(
xScale: ScaleBand,
xScale: ScaleBand<string | number>,
x0: PrimitiveValue,
x1: PrimitiveValue,
): { x: number; width: number } | null {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export function computeAnnotationDimensions(
annotations: AnnotationSpec[],
chartRotation: Rotation,
yScales: Map<GroupId, Scale<number>>,
xScale: Scale<unknown>,
xScale: Scale<string | number>,
axesSpecs: AxisSpec[],
isHistogramModeEnabled: boolean,
smallMultipleScales: SmallMultipleScales,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export const computeAnnotationDimensionsSelector = createCustomCachedSelector(
annotationSpecs,
settingsSpec.rotation,
yScales as Map<GroupId, Scale<number>>,
xScale,
xScale as Scale<string | number>,
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
axesSpecs,
isHistogramMode,
smallMultipleScales,
Expand Down
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<string | number>,
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
8 changes: 5 additions & 3 deletions packages/charts/src/scales/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,18 @@ export type ScaleBandType = ScaleOrdinalType;
* The the value is mapped depending on the `type` (linear, log, sqrt, time, ordinal)
* @internal
*/
export interface Scale<D> {
domain: D[];
export interface Scale<T> {
domain: T[];
range: number[];
/**
* Returns the distance between the starts of adjacent bands.
*/
step: number;
ticks: () => number[];
ticks: () => T[];
monfera marked this conversation as resolved.
Show resolved Hide resolved
scale: (value?: PrimitiveValue) => number | null;

scaleOrThrow(value?: PrimitiveValue): number;

pureScale: (value?: PrimitiveValue) => number | null;
invert: (value: number) => any;
invertWithStep: (
Expand Down
6 changes: 3 additions & 3 deletions packages/charts/src/scales/scale_band.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ export class ScaleBand<T extends number | string> implements Scale<T> {

readonly type: ScaleBandType;

readonly domain: any[];
readonly domain: T[];

readonly range: number[];

readonly isInverted: boolean;

readonly invertedScale: ScaleQuantize<number>;
readonly invertedScale: ScaleQuantize<T>;

readonly minInterval: number;

Expand Down Expand Up @@ -89,7 +89,7 @@ export class ScaleBand<T extends number | string> implements Scale<T> {
this.bandwidthPadding = this.bandwidth;
// TO FIX: we are assuming that it's ordered
this.isInverted = this.domain[0] > this.domain[1];
this.invertedScale = scaleQuantize().domain(range).range(this.domain);
this.invertedScale = scaleQuantize<T>().domain(range).range(this.domain);
this.minInterval = 0;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/charts/src/scales/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export function isLogarithmicScale(scale: Scale<unknown>): scale is ScaleContinu
* Check if a scale is Band
* @internal
*/
export function isBandScale(scale: Scale<unknown>): scale is ScaleBand {
export function isBandScale(scale: Scale<unknown>): scale is ScaleBand<string | number> {
monfera marked this conversation as resolved.
Show resolved Hide resolved
return scale.type === ScaleType.Ordinal;
}

Expand Down