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

Jump to bottom

feat: nice ticks everywhere #1087

Merged
merged 18 commits into from
Apr 13, 2021
Merged

feat: nice ticks everywhere #1087

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
fe47338
feat: nice ticks everywhere
markov00 Mar 23, 2021
acee82b
feat: add nice prop on scale type
markov00 Mar 29, 2021
521a15f
refactor: compute scale API config separately
markov00 Mar 31, 2021
5aa72fc
test: fix wrong test change
markov00 Apr 1, 2021
6ec6b3e
fix: reduce global scale correctly
markov00 Apr 6, 2021
5d2f1a8
fix: stories
markov00 Apr 6, 2021
c063d22
Merge branch 'master' into 2021_03_23-nice_ticks_everywhere
markov00 Apr 6, 2021
08527a0
chore: fix lint and api docs
markov00 Apr 7, 2021
5120ef8
Merge branch 'master' into 2021_03_23-nice_ticks_everywhere
markov00 Apr 7, 2021
b9db40c
fix: review comments
markov00 Apr 7, 2021
38ca96f
Merge branch '2021_03_23-nice_ticks_everywhere' of github.com:markov0…
markov00 Apr 7, 2021
e0416b5
Merge branch 'master' into 2021_03_23-nice_ticks_everywhere
markov00 Apr 7, 2021
cedf1bb
refactor: rename internal ticks in desiredTickCount
markov00 Apr 8, 2021
c0060d0
refactor: split type and nice into separate props
markov00 Apr 12, 2021
f0a9090
Merge branch '2021_03_23-nice_ticks_everywhere' of github.com:markov0…
markov00 Apr 12, 2021
7062f47
Merge branch 'master' into 2021_03_23-nice_ticks_everywhere
markov00 Apr 12, 2021
2d16951
test: update VRT with negative values
markov00 Apr 12, 2021
df26747
Merge branch 'master' into 2021_03_23-nice_ticks_everywhere
markov00 Apr 13, 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
4 changes: 0 additions & 4 deletions .playground/playground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ import React from 'react';

import { Chart, AreaSeries, LineSeries, BarSeries, ScaleType } from '../src';

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
14 changes: 4 additions & 10 deletions api/charts.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,6 @@ export const AnnotationType: Readonly<{
// @public (undocumented)
export type AnnotationType = $Values<typeof AnnotationType>;

// @public (undocumented)
export interface APIScale<T extends ScaleType> {
// (undocumented)
nice: boolean;
// (undocumented)
type: T;
}

// @public (undocumented)
export interface ArcSeriesStyle {
// (undocumented)
Expand Down Expand Up @@ -1685,10 +1677,12 @@ 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 | APIScale<XScaleType>;
xNice?: boolean;
xScaleType: XScaleType;
yNice?: boolean;
// @deprecated
yScaleToDataExtent?: boolean;
yScaleType: ScaleContinuousType | APIScale<ScaleContinuousType>;
yScaleType: ScaleContinuousType;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏼


// @public (undocumented)
Expand Down
Binary file modified ...sual-tests-for-all-stories-axes-custom-domain-visually-looks-correct-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion src/chart_types/heatmap/layout/viewmodel/viewmodel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export function shapeViewModel(
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),
desiredTickCount: getTicks(chartDimensions.width, config.xAxisLabel),
timeZone: config.timeZone,
},
)
Expand Down
2 changes: 1 addition & 1 deletion src/chart_types/heatmap/specs/scale_defaults.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ import { ScaleType } from '../../../scales/constants';
export const X_SCALE_DEFAULT = {
type: ScaleType.Ordinal,
nice: false,
ticks: 10,
desiredTickCount: 10,
};
7 changes: 4 additions & 3 deletions src/chart_types/heatmap/state/selectors/get_heatmap_table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ 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 { getXNiceFromSpec, getXScaleTypeFromSpec } 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 @@ -77,9 +77,10 @@ export const getHeatmapTableSelector = createCachedSelector(

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),
type: getXScaleTypeFromSpec(spec.xScaleType),
nice: getXNiceFromSpec(),
isBandScale: false,
ticks: X_SCALE_DEFAULT.ticks,
desiredTickCount: X_SCALE_DEFAULT.desiredTickCount,
customDomain: xDomain,
},
resultData.xValues,
Expand Down
2 changes: 0 additions & 2 deletions src/chart_types/specs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ 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
12 changes: 6 additions & 6 deletions src/chart_types/xy_chart/crosshair/crosshair_utils.linear_snap.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ 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 { getScaleConfigsFromSpecs } 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 @@ -103,31 +103,31 @@ describe('Crosshair utils linear scale', () => {
const barSeries = [barSeries1];
const barSeriesDomains = computeSeriesDomains(
barSeries,
getAPIScaleConfigs([], barSeries, MockGlobalSpec.settings()),
getScaleConfigsFromSpecs([], barSeries, MockGlobalSpec.settings()),
);

const multiBarSeries = [barSeries1, barSeries2];
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()),
getScaleConfigsFromSpecs([], multiBarSeries, MockGlobalSpec.settings()),
);

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

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

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

const barSeriesScale = computeXScale({
Expand Down
12 changes: 6 additions & 6 deletions src/chart_types/xy_chart/crosshair/crosshair_utils.ordinal_snap.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ 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 { getScaleConfigsFromSpecs } 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 @@ -102,31 +102,31 @@ describe('Crosshair utils ordinal scales', () => {

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

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

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

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

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

const barSeriesScale = computeXScale({
Expand Down
39 changes: 20 additions & 19 deletions src/chart_types/xy_chart/domains/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,28 @@ import { ScaleContinuousType } from '../../../scales';
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 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;
};
export type XDomain = Pick<LogScaleOptions, 'logBase'> & {
type: XScaleType;
nice: boolean;
/* 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;
desiredTickCount: number;
};

/** @internal */
export type YDomain = LogScaleOptions &
APIScale<ScaleContinuousType> & {
isBandScale: false;
groupId: GroupId;
domain: ContinuousDomain;
ticks: number;
};
export type YDomain = LogScaleOptions & {
type: ScaleContinuousType;
nice: boolean;
isBandScale: false;
groupId: GroupId;
domain: ContinuousDomain;
desiredTickCount: number;
};
Loading