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: nice ticks everywhere #1087

Merged
merged 18 commits into from
Apr 13, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
1 change: 1 addition & 0 deletions .playground/playground.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { Chart, BarSeries, ScaleType, LineAnnotation, AnnotationDomainTypes, Lin
function generateAnnotationData(values: any[]): LineAnnotationDatum[] {
return values.map((value, index) => ({ dataValue: value, details: `detail-${index}` }));
}

export class Playground extends React.Component {
render() {
return (
Expand Down
12 changes: 10 additions & 2 deletions api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ export const AnnotationType: Readonly<{
// @public (undocumented)
export type AnnotationType = $Values<typeof AnnotationType>;

// @public (undocumented)
export interface APIScale<T extends ScaleType> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure of the pros/cons of putting the word API in an element of the API, all things being equal, would prefer a simple, expressive, semantic name. I'd probably not call it Scale because it's currently just a tuple of {type: 'linear' | ..., nice: boolean}

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, any suggestion? what if we don't expose the interface externally on our API, and we reserve the APIScaletype for internal purposes?
The external will look like this:


export interface SeriesScales {
    ...
    xScaleType: XScaleType | { nice: boolean; type: XScaleType };
    yScaleType: ScaleContinuousType | { nice: boolean; type: ScaleContinuousType };
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, even for internal use the word "API" in it is a good question, maybe it has a bunch of benefits, eg. knowing which "boundary layer" we talk about, eg. external API, internal API or implementation detail (the latter of which is clear from @internal).

Could namespaces / modules achieve this without resorting to Hungarian notation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this inception or recursion?

Copy link
Contributor

Choose a reason for hiding this comment

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

image

// (undocumented)
nice: boolean;
// (undocumented)
type: T;
}
monfera marked this conversation as resolved.
Show resolved Hide resolved

// @public (undocumented)
export interface ArcSeriesStyle {
// (undocumented)
Expand Down Expand Up @@ -1669,10 +1677,10 @@ export type SeriesNameFn = (series: XYChartSeriesIdentifier, isTooltip: boolean)
// @public (undocumented)
export interface SeriesScales {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to the PR:

  • instead of SeriesScales, we might eventually consider a more specific term, eg. ProjectionPair or CartesianPlaneProjections (or a shorter version), to express that it's not some set or array of scales, but it defines the Cartesian plane, with the orthogonal projection pair
  • also, "projection" might work better here, and also in a lot of places where we use scale at the moment; Projection captures the linearity (or how it's non-linear) of a dimension, eg. current ScaleType; while Scale adds the multiplier and usually offset, for example, by using the data domain and the screen pixel range

timeZone?: string;
xScaleType: XScaleType;
xScaleType: XScaleType | APIScale<XScaleType>;
// @deprecated
yScaleToDataExtent?: boolean;
yScaleType: ScaleContinuousType;
yScaleType: ScaleContinuousType | APIScale<ScaleContinuousType>;
}

// @public (undocumented)
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
5 changes: 3 additions & 2 deletions src/chart_types/heatmap/layout/viewmodel/viewmodel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,13 @@ export function shapeViewModel(
let xValues = xDomain.domain as any[];

const timeScale =
xDomain.scaleType === ScaleType.Time
xDomain.type === ScaleType.Time
Copy link
Contributor

Choose a reason for hiding this comment

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

The word domain covers, or maybe in our case, characterizes the set of values that a variable can assume, and is only very loosely connected to the projection type which is what we mean here, eg. linear, logarithmic. Not just in math, but also in geography and eg. D3, domain and projection are quite independent.

This has practical use too. For example, it'll be eventually useful to describe/configure the domain as a first class entity (and codomain, which we can call yDomain), and share it among multiple visualizations, some of which may use linear projection, some, logarithmic projection, and some, square root projection (eg. for varying size heatmap squares).

Domains compose nicely, eg. the discussed operators, like

  • unioning (not just enlarging the domain if the user wants, but also, projecting multiple series on the same cartesian dimesion, we need to compute the union of the domains of all the series)
  • mandatory inclusion of a certain obligatevalue, eg. zero—which is of course just a special case of unioning (unioning with a set that has a singluar value)
  • intersection of the domain, eg. crop outliers (maybe shown or listed separately or on an overview chart) etc

Copy link
Member Author

Choose a reason for hiding this comment

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

we can clean this up in a different PR.
We should change not only the variable name, but also the enum

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's good, do you have a list of items on which this can be put, to eventually circle back if deemed useful?

? new ScaleContinuous(
{
type: ScaleType.Time,
domain: xDomain.domain,
range: [0, chartDimensions.width],
nice: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the nice flag forms high cohesion with the domain of the data and the pixel range onto which we project, as these are equally needed to compute the ticks. Also, the proposed tick count, or tick density (eg. via max font height and padding, avoiding overlap or too many/too few ticks)

},
{
ticks: getTicks(chartDimensions.width, config.xAxisLabel),
Expand Down Expand Up @@ -315,7 +316,7 @@ export function shapeViewModel(
* @param y
*/
const pickHighlightedArea: PickHighlightedArea = (x: Array<string | number>, y: Array<string | number>) => {
if (xDomain.scaleType !== ScaleType.Time) {
if (xDomain.type !== ScaleType.Time) {
return null;
}
const [startValue, endValue] = x;
Expand Down
3 changes: 2 additions & 1 deletion src/chart_types/heatmap/specs/heatmap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { Accessor, AccessorFn } from '../../../utils/accessor';
import { Color, Datum, RecursivePartial } from '../../../utils/common';
import { config } from '../layout/config/config';
import { Config } from '../layout/types/config_types';
import { X_SCALE_DEFAULT } from './scale_defaults';

const defaultProps = {
chartType: ChartType.Heatmap,
Expand All @@ -38,7 +39,7 @@ const defaultProps = {
colorScale: ScaleType.Linear,
xAccessor: ({ x }: { x: string | number }) => x,
yAccessor: ({ y }: { y: string | number }) => y,
xScaleType: ScaleType.Ordinal,
xScaleType: X_SCALE_DEFAULT.type,
valueAccessor: ({ value }: { value: string | number }) => value,
valueFormatter: (value: number) => `${value}`,
xSortPredicate: Predicate.AlphaAsc,
Expand Down
27 changes: 27 additions & 0 deletions src/chart_types/heatmap/specs/scale_defaults.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* 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 { ScaleType } from '../../../scales/constants';

/** @internal */
export const X_SCALE_DEFAULT = {
type: ScaleType.Ordinal,
nice: false,
ticks: 10,
monfera marked this conversation as resolved.
Show resolved Hide resolved
};
12 changes: 11 additions & 1 deletion src/chart_types/heatmap/state/selectors/get_heatmap_table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { getAccessorValue } from '../../../../utils/accessor';
import { mergeXDomain } from '../../../xy_chart/domains/x_domain';
import { getDefaultAPIScale } from '../../../xy_chart/scales/get_api_scales';
import { X_SCALE_DEFAULT } from '../../specs/scale_defaults';
import { HeatmapTable } from './compute_chart_dimensions';
import { getHeatmapSpecSelector } from './get_heatmap_spec';

Expand Down Expand Up @@ -73,7 +75,15 @@ export const getHeatmapTableSelector = createCachedSelector(
},
);

resultData.xDomain = mergeXDomain([{ xScaleType: spec.xScaleType }], resultData.xValues, xDomain);
resultData.xDomain = mergeXDomain(
Copy link
Contributor

Choose a reason for hiding this comment

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

mergeXDomain appears to union the domain and do other things, maybe could be split to compose from a bunch of smaller functions

{
...getDefaultAPIScale(spec.xScaleType, X_SCALE_DEFAULT),
isBandScale: false,
ticks: X_SCALE_DEFAULT.ticks,
customDomain: xDomain,
},
resultData.xValues,
);

// sort values by their predicates
if (spec.xScaleType === ScaleType.Ordinal) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { getHeatmapTableSelector } from './get_heatmap_table';
export const getXAxisRightOverflow = createCachedSelector(
[getHeatmapConfigSelector, getHeatmapTableSelector],
({ xAxisLabel: { fontSize, fontFamily, padding, formatter, width }, timeZone }, { xDomain }): number => {
if (xDomain.scaleType !== ScaleType.Time) {
if (xDomain.type !== ScaleType.Time) {
return 0;
}
if (typeof width === 'number') {
Expand Down
2 changes: 2 additions & 0 deletions src/chart_types/specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export {
RectAnnotation,
} from './xy_chart/specs';

export { APIScale } from './xy_chart/scales/types';

export * from './xy_chart/utils/specs';

export { Partition } from './partition_chart/specs';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@
*/

import { ChartType } from '../..';
import { MockGlobalSpec } from '../../../mocks/specs/specs';
import { MockXDomain } from '../../../mocks/xy/domains';
import { ScaleType } from '../../../scales/constants';
import { SpecType } from '../../../specs/constants';
import { Dimensions } from '../../../utils/dimensions';
import { getAPIScaleConfigs } from '../state/selectors/get_api_scale_configs';
import { computeSeriesDomains } from '../state/utils/utils';
import { computeXScale } from '../utils/scales';
import { BasicSeriesSpec, SeriesType } from '../utils/specs';
Expand Down Expand Up @@ -97,22 +100,35 @@ describe('Crosshair utils linear scale', () => {
yScaleType: ScaleType.Linear,
};

const domainGroup = new Map([['group1', { fit: true }]]);

const barSeries = [barSeries1];
const barSeriesDomains = computeSeriesDomains(barSeries, domainGroup);
const barSeriesDomains = computeSeriesDomains(
barSeries,
getAPIScaleConfigs([], barSeries, MockGlobalSpec.settings()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's not the right PR to discuss it, though here there's an addition. I've been thinking about the word get in function names. Some advisory suggests not using get, calculate etc. as these are typically all functions, just like the others, that calculate and get you something. So maybe it could lose the get, or if we want to reflect on where it's retrieved from, it could be seriesScaleConfigs or some such

);

const multiBarSeries = [barSeries1, barSeries2];
const multiBarSeriesDomains = computeSeriesDomains(multiBarSeries, domainGroup);
const multiBarSeriesDomains = computeSeriesDomains(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with compute - maybe there's a more descriptive word, though it's better if the function name is just a reflection of the returned value. Eg. seriesDomains or cartesianPlaneDomains or similar

multiBarSeries,
getAPIScaleConfigs([], multiBarSeries, MockGlobalSpec.settings()),
);

const lineSeries = [lineSeries1];
const lineSeriesDomains = computeSeriesDomains(lineSeries, domainGroup);
const lineSeriesDomains = computeSeriesDomains(
lineSeries,
getAPIScaleConfigs([], lineSeries, MockGlobalSpec.settings()),
);

const multiLineSeries = [lineSeries1, lineSeries2];
const multiLineSeriesDomains = computeSeriesDomains(multiLineSeries, domainGroup);
const multiLineSeriesDomains = computeSeriesDomains(
multiLineSeries,
getAPIScaleConfigs([], multiLineSeries, MockGlobalSpec.settings()),
);

const mixedLinesBars = [lineSeries1, lineSeries2, barSeries1, barSeries2];
const mixedLinesBarsSeriesDomains = computeSeriesDomains(mixedLinesBars, domainGroup);
const mixedLinesBarsSeriesDomains = computeSeriesDomains(
mixedLinesBars,
getAPIScaleConfigs([], mixedLinesBars, MockGlobalSpec.settings()),
);

const barSeriesScale = computeXScale({
xDomain: barSeriesDomains.xDomain,
Expand Down Expand Up @@ -1456,13 +1472,11 @@ describe('Crosshair utils linear scale', () => {
const chartDimensions: Dimensions = { top: 0, left: 0, width: 120, height: 120 };
test('cursor at begin of domain', () => {
const barSeriesScaleLimited = computeXScale({
xDomain: {
xDomain: MockXDomain.fromScaleType(ScaleType.Linear, {
domain: [0.5, 3.5],
isBandScale: true,
minInterval: 1,
scaleType: ScaleType.Linear,
type: 'xDomain',
},
}),
totalBarsInCluster: 1,
range: [0, 120],
});
Expand All @@ -1487,13 +1501,11 @@ describe('Crosshair utils linear scale', () => {
});
test('cursor at end of domain', () => {
const barSeriesScaleLimited = computeXScale({
xDomain: {
xDomain: MockXDomain.fromScaleType(ScaleType.Linear, {
domain: [-0.5, 2.5],
isBandScale: true,
minInterval: 1,
scaleType: ScaleType.Linear,
type: 'xDomain',
},
}),
totalBarsInCluster: barSeries.length,
range: [0, 120],
});
Expand All @@ -1518,13 +1530,11 @@ describe('Crosshair utils linear scale', () => {
});
test('cursor at top begin of domain', () => {
const barSeriesScaleLimited = computeXScale({
xDomain: {
xDomain: MockXDomain.fromScaleType(ScaleType.Linear, {
domain: [0.5, 3.5],
isBandScale: true,
minInterval: 1,
scaleType: ScaleType.Linear,
type: 'xDomain',
},
}),
totalBarsInCluster: 1,
range: [0, 120],
});
Expand All @@ -1549,13 +1559,11 @@ describe('Crosshair utils linear scale', () => {
});
test('cursor at top end of domain', () => {
const barSeriesScaleLimited = computeXScale({
xDomain: {
xDomain: MockXDomain.fromScaleType(ScaleType.Linear, {
domain: [-0.5, 2.5],
isBandScale: true,
minInterval: 1,
scaleType: ScaleType.Linear,
type: 'xDomain',
},
}),
totalBarsInCluster: barSeries.length,
range: [0, 120],
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
*/

import { ChartType } from '../..';
import { MockGlobalSpec } from '../../../mocks/specs/specs';
import { ScaleType } from '../../../scales/constants';
import { SpecType } from '../../../specs/constants';
import { getAPIScaleConfigs } from '../state/selectors/get_api_scale_configs';
import { computeSeriesDomains } from '../state/utils/utils';
import { computeXScale } from '../utils/scales';
import { BasicSeriesSpec, SeriesType } from '../utils/specs';
Expand Down Expand Up @@ -96,22 +98,36 @@ describe('Crosshair utils ordinal scales', () => {
yScaleType: ScaleType.Linear,
};

const domainGroup = new Map([['group1', { fit: true }]]);

const barSeries = [barSeries1];
const barSeriesDomains = computeSeriesDomains(barSeries, domainGroup);

const barSeriesDomains = computeSeriesDomains(
barSeries,
getAPIScaleConfigs([], barSeries, MockGlobalSpec.settings()),
);

const multiBarSeries = [barSeries1, barSeries2];
const multiBarSeriesDomains = computeSeriesDomains(multiBarSeries, domainGroup);
const multiBarSeriesDomains = computeSeriesDomains(
multiBarSeries,
getAPIScaleConfigs([], multiBarSeries, MockGlobalSpec.settings()),
);

const lineSeries = [lineSeries1];
const lineSeriesDomains = computeSeriesDomains(lineSeries, domainGroup);
const lineSeriesDomains = computeSeriesDomains(
lineSeries,
getAPIScaleConfigs([], lineSeries, MockGlobalSpec.settings()),
);

const multiLineSeries = [lineSeries1, lineSeries2];
const multiLineSeriesDomains = computeSeriesDomains(multiLineSeries, domainGroup);
const multiLineSeriesDomains = computeSeriesDomains(
multiLineSeries,
getAPIScaleConfigs([], multiLineSeries, MockGlobalSpec.settings()),
);

const mixedLinesBars = [lineSeries1, lineSeries2, barSeries1, barSeries2];
const mixedLinesBarsSeriesDomains = computeSeriesDomains(mixedLinesBars, domainGroup);
const mixedLinesBarsSeriesDomains = computeSeriesDomains(
mixedLinesBars,
getAPIScaleConfigs([], mixedLinesBars, MockGlobalSpec.settings()),
);

const barSeriesScale = computeXScale({
xDomain: barSeriesDomains.xDomain,
Expand Down
23 changes: 23 additions & 0 deletions src/chart_types/xy_chart/domains/nice.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* 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.
*/

/** @internal */
export function areAllNiceDomain(nice: Array<boolean>) {
return nice.length > 0 && nice.every((d) => d);
}
25 changes: 10 additions & 15 deletions src/chart_types/xy_chart/domains/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,30 @@
*/

import { ScaleContinuousType } from '../../../scales';
import { ScaleType } from '../../../scales/constants';
import { LogScaleOptions } from '../../../scales/scale_continuous';
import { OrdinalDomain, ContinuousDomain } from '../../../utils/domain';
import { GroupId } from '../../../utils/ids';
import { APIScale } from '../scales/types';
import { XScaleType } from '../utils/specs';

/** @internal */
export interface BaseDomain {
scaleType: typeof ScaleType.Ordinal | ScaleContinuousType;
/* if the scale needs to be a band scale: used when displaying bars */
isBandScale: boolean;
}

/** @internal */
export type XDomain = BaseDomain &
Pick<LogScaleOptions, 'logBase'> & {
type: 'xDomain';
export type XDomain = Pick<LogScaleOptions, 'logBase'> &
APIScale<XScaleType> & {
/* if the scale needs to be a band scale: used when displaying bars */
isBandScale: boolean;
/* the minimum interval of the scale if not-ordinal band-scale */
minInterval: number;
/** if x domain is time, we should also specify the timezone */
timeZone?: string;
domain: OrdinalDomain | ContinuousDomain;
ticks: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about tickCount if precise, or desiredTickCount or tickCountTarget (or whatever rolls off the tongue) if it's just a wish

};

/** @internal */
export type YDomain = BaseDomain &
LogScaleOptions & {
type: 'yDomain';
export type YDomain = LogScaleOptions &
APIScale<ScaleContinuousType> & {
isBandScale: false;
scaleType: ScaleContinuousType;
groupId: GroupId;
domain: ContinuousDomain;
ticks: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

same

};
Loading