-
Notifications
You must be signed in to change notification settings - Fork 120
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
Changes from 12 commits
fe47338
acee82b
521a15f
5aa72fc
6ec6b3e
5d2f1a8
c063d22
08527a0
5120ef8
b9db40c
38ca96f
e0416b5
cedf1bb
c0060d0
f0a9090
7062f47
2d16951
df26747
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,14 @@ 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; | ||
} | ||
monfera marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// @public (undocumented) | ||
export interface ArcSeriesStyle { | ||
// (undocumented) | ||
|
@@ -1669,10 +1677,10 @@ export type SeriesNameFn = (series: XYChartSeriesIdentifier, isTooltip: boolean) | |
// @public (undocumented) | ||
export interface SeriesScales { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to the PR:
|
||
timeZone?: string; | ||
xScaleType: XScaleType; | ||
xScaleType: XScaleType | APIScale<XScaleType>; | ||
// @deprecated | ||
yScaleToDataExtent?: boolean; | ||
yScaleType: ScaleContinuousType; | ||
yScaleType: ScaleContinuousType | APIScale<ScaleContinuousType>; | ||
} | ||
|
||
// @public (undocumented) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,12 +110,13 @@ export function shapeViewModel( | |
let xValues = xDomain.domain as any[]; | ||
|
||
const timeScale = | ||
xDomain.scaleType === ScaleType.Time | ||
xDomain.type === ScaleType.Time | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can clean this up in a different PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the |
||
}, | ||
{ | ||
ticks: getTicks(chartDimensions.width, config.xAxisLabel), | ||
|
@@ -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; | ||
|
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
|
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -73,7 +75,15 @@ export const getHeatmapTableSelector = createCachedSelector( | |
}, | ||
); | ||
|
||
resultData.xDomain = mergeXDomain([{ xScaleType: spec.xScaleType }], resultData.xValues, xDomain); | ||
resultData.xDomain = mergeXDomain( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
...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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
); | ||
|
||
const multiBarSeries = [barSeries1, barSeries2]; | ||
const multiBarSeriesDomains = computeSeriesDomains(multiBarSeries, domainGroup); | ||
const multiBarSeriesDomains = computeSeriesDomains( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same with |
||
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, | ||
|
@@ -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], | ||
}); | ||
|
@@ -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], | ||
}); | ||
|
@@ -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], | ||
}); | ||
|
@@ -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], | ||
}); | ||
|
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); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about |
||
}; | ||
|
||
/** @internal */ | ||
export type YDomain = BaseDomain & | ||
LogScaleOptions & { | ||
type: 'yDomain'; | ||
export type YDomain = LogScaleOptions & | ||
APIScale<ScaleContinuousType> & { | ||
isBandScale: false; | ||
scaleType: ScaleContinuousType; | ||
groupId: GroupId; | ||
domain: ContinuousDomain; | ||
ticks: number; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
}; |
There was a problem hiding this comment.
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 itScale
because it's currently just a tuple of{type: 'linear' | ..., nice: boolean}
There was a problem hiding this comment.
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
APIScale
type for internal purposes?The external will look like this:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.