From a8be45bf3ed6768440382dee4d480b3bc097b6be Mon Sep 17 00:00:00 2001 From: Emma Cunningham Date: Thu, 21 Mar 2019 11:58:04 -0700 Subject: [PATCH 1/8] feat(custom domain): use computed min/max if missing on custom --- src/lib/series/domains/x_domain.ts | 6 +++++- src/lib/series/domains/y_domain.ts | 10 +++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/lib/series/domains/x_domain.ts b/src/lib/series/domains/x_domain.ts index 4fffbee487..92ac8e3ef2 100644 --- a/src/lib/series/domains/x_domain.ts +++ b/src/lib/series/domains/x_domain.ts @@ -45,7 +45,11 @@ export function mergeXDomain( if (xDomain.min > xDomain.max) { throw new Error('custom xDomain is invalid, min is greater than max'); } - seriesXComputedDomains = [xDomain.min, xDomain.max]; + + const [computedDomainMin, computedDomainMax] = seriesXComputedDomains; + const domainMin = xDomain.min == null ? computedDomainMin : xDomain.min; + const domainMax = xDomain.max == null ? computedDomainMax : xDomain.max; + seriesXComputedDomains = [domainMin, domainMax]; } else { throw new Error('xDomain for continuous scale should be a DomainRange object, not an array'); } diff --git a/src/lib/series/domains/y_domain.ts b/src/lib/series/domains/y_domain.ts index da1e06570a..ba63380c51 100644 --- a/src/lib/series/domains/y_domain.ts +++ b/src/lib/series/domains/y_domain.ts @@ -63,8 +63,16 @@ export function mergeYDomain( isStackedScaleToExtent || isNonStackedScaleToExtent, ); + let domainMin = groupDomain[0]; + let domainMax = groupDomain[1]; + const customDomain = domainsByGroupId.get(groupId); - const domain = customDomain ? [customDomain.min, customDomain.max] : groupDomain; + + if (customDomain) { + domainMin = customDomain.min; + domainMax = customDomain.max; + } + const domain = [domainMin, domainMax]; return { type: 'yDomain', From d2b2b75d1757bd85c2b9f54ea605bbdc6d4aa29c Mon Sep 17 00:00:00 2001 From: Emma Cunningham Date: Fri, 22 Mar 2019 10:12:13 -0700 Subject: [PATCH 2/8] feat(domain): allow partially bounded custom domain --- src/lib/axes/axis_utils.ts | 31 +++++++++++++++++++++++------- src/lib/series/domains/x_domain.ts | 6 +++--- src/lib/series/domains/y_domain.ts | 10 +++++++--- src/lib/series/specs.ts | 23 +++++++++++++++++++++- src/state/chart_state.ts | 4 ++-- src/state/utils.ts | 6 +++--- 6 files changed, 61 insertions(+), 19 deletions(-) diff --git a/src/lib/axes/axis_utils.ts b/src/lib/axes/axis_utils.ts index 50eb38fb72..eaa32edf4b 100644 --- a/src/lib/axes/axis_utils.ts +++ b/src/lib/axes/axis_utils.ts @@ -1,7 +1,7 @@ import { XDomain } from '../series/domains/x_domain'; import { YDomain } from '../series/domains/y_domain'; import { computeXScale, computeYScales } from '../series/scales'; -import { AxisSpec, DomainRange, Position, Rotation, TickFormatter } from '../series/specs'; +import { AxisSpec, PartialDomainRange, Position, Rotation, TickFormatter } from '../series/specs'; import { AxisConfig, Theme } from '../themes/theme'; import { Dimensions, Margins } from '../utils/dimensions'; import { Domain } from '../utils/domain'; @@ -623,11 +623,13 @@ export function isHorizontal(position: Position) { export function mergeDomainsByGroupId( axesSpecs: Map, chartRotation: Rotation, -): Map { - const domainsByGroupId = new Map(); +): Map { + const domainsByGroupId = new Map(); axesSpecs.forEach((spec: AxisSpec, id: AxisId) => { - const { groupId, domain } = spec; + const { groupId } = spec; + + const domain = spec.domain as PartialDomainRange; if (!domain) { return; @@ -640,7 +642,7 @@ export function mergeDomainsByGroupId( throw new Error(errorMessage); } - if (domain.min > domain.max) { + if ((domain.min != null && domain.max != null) && domain.min > domain.max) { const errorMessage = `[Axis ${id}]: custom domain is invalid, min is greater than max`; throw new Error(errorMessage); } @@ -648,9 +650,24 @@ export function mergeDomainsByGroupId( const prevGroupDomain = domainsByGroupId.get(groupId); if (prevGroupDomain) { + let max; + let min; + + const prevMin = prevGroupDomain.min; + const prevMax = prevGroupDomain.max; + + if ((domain.min != null && domain.max != null)) { + min = prevMin ? Math.min(domain.min, prevMin) : domain.min; + max = prevMax ? Math.max(domain.max, prevMax) : domain.max; + } else if (domain.min != null) { + min = prevMin ? Math.min(domain.min, prevMin) : domain.min; + } else if (domain.max != null) { + max = prevMax ? Math.max(domain.max, prevMax) : domain.max; + } + const mergedDomain = { - min: Math.min(domain.min, prevGroupDomain.min), - max: Math.max(domain.max, prevGroupDomain.max), + min, + max, }; domainsByGroupId.set(groupId, mergedDomain); diff --git a/src/lib/series/domains/x_domain.ts b/src/lib/series/domains/x_domain.ts index 92ac8e3ef2..409847d858 100644 --- a/src/lib/series/domains/x_domain.ts +++ b/src/lib/series/domains/x_domain.ts @@ -1,7 +1,7 @@ import { compareByValueAsc, identity } from '../../utils/commons'; import { computeContinuousDataDomain, computeOrdinalDataDomain, Domain } from '../../utils/domain'; import { ScaleType } from '../../utils/scales/scales'; -import { BasicSeriesSpec, DomainRange } from '../specs'; +import { BasicSeriesSpec, PartialDomainRange } from '../specs'; import { BaseDomain } from './domain'; export type XDomain = BaseDomain & { @@ -18,7 +18,7 @@ export type XDomain = BaseDomain & { export function mergeXDomain( specs: Array>, xValues: Set, - xDomain?: DomainRange | Domain, + xDomain?: PartialDomainRange | Domain, ): XDomain { const mainXScaleType = convertXScaleTypes(specs); if (!mainXScaleType) { @@ -42,7 +42,7 @@ export function mergeXDomain( seriesXComputedDomains = computeContinuousDataDomain(values, identity, true); if (xDomain) { if (!Array.isArray(xDomain)) { - if (xDomain.min > xDomain.max) { + if ((xDomain.min != null && xDomain.max != null) && xDomain.min > xDomain.max) { throw new Error('custom xDomain is invalid, min is greater than max'); } diff --git a/src/lib/series/domains/y_domain.ts b/src/lib/series/domains/y_domain.ts index ba63380c51..207f409072 100644 --- a/src/lib/series/domains/y_domain.ts +++ b/src/lib/series/domains/y_domain.ts @@ -4,7 +4,7 @@ import { computeContinuousDataDomain } from '../../utils/domain'; import { GroupId, SpecId } from '../../utils/ids'; import { ScaleContinuousType, ScaleType } from '../../utils/scales/scales'; import { RawDataSeries } from '../series'; -import { BasicSeriesSpec, DomainRange } from '../specs'; +import { BasicSeriesSpec, PartialDomainRange } from '../specs'; import { BaseDomain } from './domain'; export type YDomain = BaseDomain & { @@ -27,7 +27,7 @@ export type YBasicSeriesSpec = Pick< export function mergeYDomain( dataSeries: Map, specs: YBasicSeriesSpec[], - domainsByGroupId: Map, + domainsByGroupId: Map, ): YDomain[] { // group specs by group ids const specsByGroupIds = splitSpecsByGroupId(specs); @@ -68,10 +68,14 @@ export function mergeYDomain( const customDomain = domainsByGroupId.get(groupId); - if (customDomain) { + if (customDomain && customDomain.min != null) { domainMin = customDomain.min; + } + + if (customDomain && customDomain.max != null) { domainMax = customDomain.max; } + const domain = [domainMin, domainMax]; return { diff --git a/src/lib/series/specs.ts b/src/lib/series/specs.ts index e975dce8a7..ec9c1948f3 100644 --- a/src/lib/series/specs.ts +++ b/src/lib/series/specs.ts @@ -9,11 +9,32 @@ export type Datum = any; export type Rotation = 0 | 90 | -90 | 180; export type Rendering = 'canvas' | 'svg'; -export interface DomainRange { +export enum DomainRangeType { + Lower, + Upper, + Complete, +} + +export interface PartialDomainRange { + min?: number; + max?: number; +} + +export interface LowerBoundedDomain { min: number; +} + +export interface UpperBoundedDomain { max: number; } +export interface CompleteBoundedDomain { + min: number; + max: number; +} + +export type DomainRange = LowerBoundedDomain | UpperBoundedDomain | CompleteBoundedDomain; + export interface SeriesSpec { /** The ID of the spec, generated via getSpecId method */ id: SpecId; diff --git a/src/state/chart_state.ts b/src/state/chart_state.ts index 5c5f8d8f63..eae9f7206f 100644 --- a/src/state/chart_state.ts +++ b/src/state/chart_state.ts @@ -32,8 +32,8 @@ import { AxisSpec, BarSeriesSpec, BasicSeriesSpec, - DomainRange, LineSeriesSpec, + PartialDomainRange, Position, Rendering, Rotation, @@ -139,7 +139,7 @@ export class ChartStore { seriesDomainsAndData?: SeriesDomainsAndData; // computed xScale?: Scale; yScales?: Map; - xDomain?: Domain | DomainRange; + xDomain?: Domain | PartialDomainRange; legendItems: Map = new Map(); highlightedLegendItemKey: IObservableValue = observable.box(null); diff --git a/src/state/utils.ts b/src/state/utils.ts index a02eb2e2c8..cd3432c6ac 100644 --- a/src/state/utils.ts +++ b/src/state/utils.ts @@ -27,8 +27,8 @@ import { AreaSeriesSpec, AxisSpec, BasicSeriesSpec, - DomainRange, LineSeriesSpec, + PartialDomainRange, Rotation, } from '../lib/series/specs'; import { ColorConfig } from '../lib/themes/theme'; @@ -112,8 +112,8 @@ export function getUpdatedCustomSeriesColors( */ export function computeSeriesDomains( seriesSpecs: Map, - domainsByGroupId: Map, - customXDomain?: DomainRange | Domain, + domainsByGroupId: Map, + customXDomain?: PartialDomainRange | Domain, selectedDataSeries?: DataSeriesColorsValues[] | null, ): SeriesDomainsAndData { const { splittedSeries, xValues, seriesColors } = getSplittedSeries( From f2d8029401254e18d55093c62a369b8dd13d53c1 Mon Sep 17 00:00:00 2001 From: Emma Cunningham Date: Fri, 22 Mar 2019 10:37:08 -0700 Subject: [PATCH 3/8] refactor(domain): remove need for extra type --- src/lib/axes/axis_utils.ts | 8 ++++---- src/lib/series/domains/x_domain.ts | 4 ++-- src/lib/series/domains/y_domain.ts | 4 ++-- src/lib/series/specs.ts | 11 ----------- src/state/chart_state.ts | 4 ++-- src/state/utils.ts | 6 +++--- 6 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/lib/axes/axis_utils.ts b/src/lib/axes/axis_utils.ts index eaa32edf4b..de150c968b 100644 --- a/src/lib/axes/axis_utils.ts +++ b/src/lib/axes/axis_utils.ts @@ -1,7 +1,7 @@ import { XDomain } from '../series/domains/x_domain'; import { YDomain } from '../series/domains/y_domain'; import { computeXScale, computeYScales } from '../series/scales'; -import { AxisSpec, PartialDomainRange, Position, Rotation, TickFormatter } from '../series/specs'; +import { AxisSpec, CompleteBoundedDomain, Position, Rotation, TickFormatter } from '../series/specs'; import { AxisConfig, Theme } from '../themes/theme'; import { Dimensions, Margins } from '../utils/dimensions'; import { Domain } from '../utils/domain'; @@ -623,13 +623,13 @@ export function isHorizontal(position: Position) { export function mergeDomainsByGroupId( axesSpecs: Map, chartRotation: Rotation, -): Map { - const domainsByGroupId = new Map(); +): Map> { + const domainsByGroupId = new Map>(); axesSpecs.forEach((spec: AxisSpec, id: AxisId) => { const { groupId } = spec; - const domain = spec.domain as PartialDomainRange; + const domain = spec.domain as Partial; if (!domain) { return; diff --git a/src/lib/series/domains/x_domain.ts b/src/lib/series/domains/x_domain.ts index 409847d858..83537d60a9 100644 --- a/src/lib/series/domains/x_domain.ts +++ b/src/lib/series/domains/x_domain.ts @@ -1,7 +1,7 @@ import { compareByValueAsc, identity } from '../../utils/commons'; import { computeContinuousDataDomain, computeOrdinalDataDomain, Domain } from '../../utils/domain'; import { ScaleType } from '../../utils/scales/scales'; -import { BasicSeriesSpec, PartialDomainRange } from '../specs'; +import { BasicSeriesSpec, CompleteBoundedDomain } from '../specs'; import { BaseDomain } from './domain'; export type XDomain = BaseDomain & { @@ -18,7 +18,7 @@ export type XDomain = BaseDomain & { export function mergeXDomain( specs: Array>, xValues: Set, - xDomain?: PartialDomainRange | Domain, + xDomain?: Partial | Domain, ): XDomain { const mainXScaleType = convertXScaleTypes(specs); if (!mainXScaleType) { diff --git a/src/lib/series/domains/y_domain.ts b/src/lib/series/domains/y_domain.ts index 207f409072..dc64b56604 100644 --- a/src/lib/series/domains/y_domain.ts +++ b/src/lib/series/domains/y_domain.ts @@ -4,7 +4,7 @@ import { computeContinuousDataDomain } from '../../utils/domain'; import { GroupId, SpecId } from '../../utils/ids'; import { ScaleContinuousType, ScaleType } from '../../utils/scales/scales'; import { RawDataSeries } from '../series'; -import { BasicSeriesSpec, PartialDomainRange } from '../specs'; +import { BasicSeriesSpec, CompleteBoundedDomain } from '../specs'; import { BaseDomain } from './domain'; export type YDomain = BaseDomain & { @@ -27,7 +27,7 @@ export type YBasicSeriesSpec = Pick< export function mergeYDomain( dataSeries: Map, specs: YBasicSeriesSpec[], - domainsByGroupId: Map, + domainsByGroupId: Map>, ): YDomain[] { // group specs by group ids const specsByGroupIds = splitSpecsByGroupId(specs); diff --git a/src/lib/series/specs.ts b/src/lib/series/specs.ts index ec9c1948f3..7345464057 100644 --- a/src/lib/series/specs.ts +++ b/src/lib/series/specs.ts @@ -9,17 +9,6 @@ export type Datum = any; export type Rotation = 0 | 90 | -90 | 180; export type Rendering = 'canvas' | 'svg'; -export enum DomainRangeType { - Lower, - Upper, - Complete, -} - -export interface PartialDomainRange { - min?: number; - max?: number; -} - export interface LowerBoundedDomain { min: number; } diff --git a/src/state/chart_state.ts b/src/state/chart_state.ts index eae9f7206f..775c29f04a 100644 --- a/src/state/chart_state.ts +++ b/src/state/chart_state.ts @@ -32,8 +32,8 @@ import { AxisSpec, BarSeriesSpec, BasicSeriesSpec, + CompleteBoundedDomain, LineSeriesSpec, - PartialDomainRange, Position, Rendering, Rotation, @@ -139,7 +139,7 @@ export class ChartStore { seriesDomainsAndData?: SeriesDomainsAndData; // computed xScale?: Scale; yScales?: Map; - xDomain?: Domain | PartialDomainRange; + xDomain?: Domain | Partial; legendItems: Map = new Map(); highlightedLegendItemKey: IObservableValue = observable.box(null); diff --git a/src/state/utils.ts b/src/state/utils.ts index cd3432c6ac..8c59b3ec88 100644 --- a/src/state/utils.ts +++ b/src/state/utils.ts @@ -27,8 +27,8 @@ import { AreaSeriesSpec, AxisSpec, BasicSeriesSpec, + CompleteBoundedDomain, LineSeriesSpec, - PartialDomainRange, Rotation, } from '../lib/series/specs'; import { ColorConfig } from '../lib/themes/theme'; @@ -112,8 +112,8 @@ export function getUpdatedCustomSeriesColors( */ export function computeSeriesDomains( seriesSpecs: Map, - domainsByGroupId: Map, - customXDomain?: PartialDomainRange | Domain, + domainsByGroupId: Map>, + customXDomain?: Partial | Domain, selectedDataSeries?: DataSeriesColorsValues[] | null, ): SeriesDomainsAndData { const { splittedSeries, xValues, seriesColors } = getSplittedSeries( From bd42f36821731c89338cc8e33ffabd206a51639d Mon Sep 17 00:00:00 2001 From: Emma Cunningham Date: Fri, 22 Mar 2019 10:43:10 -0700 Subject: [PATCH 4/8] docs(axis): add story for single bound custom domain --- stories/axis.tsx | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/stories/axis.tsx b/stories/axis.tsx index b664770f66..04c776799c 100644 --- a/stories/axis.tsx +++ b/stories/axis.tsx @@ -433,15 +433,6 @@ storiesOf('Axis', module) data={[{ x: 'a', y: 2 }, { x: 'b', y: 7 }, { x: 'c', y: 3 }, { x: 'd', y: 6 }]} yScaleToDataExtent={false} /> - {/* */} ); + }) + .add('customizing domain limits [only one bound defined]', () => { + const leftDomain = { + min: number('left min', 0), + }; + + const xDomain = { + max: number('xDomain max', 3), + }; + + return ( + + + + Number(d).toFixed(2)} + domain={leftDomain} + /> + + + ); }); From 676d08b6e6a3a0b549477ef1d8af3199055e5706 Mon Sep 17 00:00:00 2001 From: Emma Cunningham Date: Fri, 22 Mar 2019 12:47:35 -0700 Subject: [PATCH 5/8] test(axis_utils): update mergeDomainsByGroupId tests for full coverage --- src/lib/axes/axis_utils.test.ts | 128 ++++++++++++++++++++++++++++++++ src/lib/axes/axis_utils.ts | 14 ++-- 2 files changed, 135 insertions(+), 7 deletions(-) diff --git a/src/lib/axes/axis_utils.test.ts b/src/lib/axes/axis_utils.test.ts index 94b5715398..dcbebd5351 100644 --- a/src/lib/axes/axis_utils.test.ts +++ b/src/lib/axes/axis_utils.test.ts @@ -1034,6 +1034,134 @@ describe('Axis computational utils', () => { ); }); + test('should merge axis domains by group id: partial upper bounded prevDomain with complete domain', () => { + const groupId = getGroupId('group_1'); + const domainRange1 = { + max: 9, + }; + + const domainRange2 = { + min: 0, + max: 7, + }; + + verticalAxisSpec.domain = domainRange1; + + const axesSpecs = new Map(); + axesSpecs.set(verticalAxisSpec.id, verticalAxisSpec); + + const axis2 = { ...verticalAxisSpec, id: getAxisId('axis2') }; + + axis2.domain = domainRange2; + axesSpecs.set(axis2.id, axis2); + + const expectedMergedMap = new Map(); + expectedMergedMap.set(groupId, { min: 0, max: 9 }); + + const mergedDomainsByGroupId = mergeDomainsByGroupId(axesSpecs, 0); + expect(mergedDomainsByGroupId).toEqual(expectedMergedMap); + }); + + test('should merge axis domains by group id: partial lower bounded prevDomain with complete domain', () => { + const groupId = getGroupId('group_1'); + const domainRange1 = { + min: -1, + }; + + const domainRange2 = { + min: 0, + max: 7, + }; + + verticalAxisSpec.domain = domainRange1; + + const axesSpecs = new Map(); + axesSpecs.set(verticalAxisSpec.id, verticalAxisSpec); + + const axis2 = { ...verticalAxisSpec, id: getAxisId('axis2') }; + + axis2.domain = domainRange2; + axesSpecs.set(axis2.id, axis2); + + const expectedMergedMap = new Map(); + expectedMergedMap.set(groupId, { min: -1, max: 7 }); + + const mergedDomainsByGroupId = mergeDomainsByGroupId(axesSpecs, 0); + expect(mergedDomainsByGroupId).toEqual(expectedMergedMap); + }); + + test('should merge axis domains by group id: partial upper bounded prevDomain with lower bounded domain', () => { + const groupId = getGroupId('group_1'); + const domainRange1 = { + max: 9, + }; + + const domainRange2 = { + min: 0, + }; + + const domainRange3 = { + min: -1, + }; + + verticalAxisSpec.domain = domainRange1; + + const axesSpecs = new Map(); + axesSpecs.set(verticalAxisSpec.id, verticalAxisSpec); + + const axis2 = { ...verticalAxisSpec, id: getAxisId('axis2') }; + + axis2.domain = domainRange2; + axesSpecs.set(axis2.id, axis2); + + const axis3 = { ...verticalAxisSpec, id: getAxisId('axis3') }; + + axis3.domain = domainRange3; + axesSpecs.set(axis3.id, axis3); + + const expectedMergedMap = new Map(); + expectedMergedMap.set(groupId, { min: -1, max: 9 }); + + const mergedDomainsByGroupId = mergeDomainsByGroupId(axesSpecs, 0); + expect(mergedDomainsByGroupId).toEqual(expectedMergedMap); + }); + + test('should merge axis domains by group id: partial lower bounded prevDomain with upper bounded domain', () => { + const groupId = getGroupId('group_1'); + const domainRange1 = { + min: 2, + }; + + const domainRange2 = { + max: 7, + }; + + const domainRange3 = { + max: 9, + }; + + verticalAxisSpec.domain = domainRange1; + + const axesSpecs = new Map(); + axesSpecs.set(verticalAxisSpec.id, verticalAxisSpec); + + const axis2 = { ...verticalAxisSpec, id: getAxisId('axis2') }; + + axis2.domain = domainRange2; + axesSpecs.set(axis2.id, axis2); + + const axis3 = { ...verticalAxisSpec, id: getAxisId('axis3') }; + + axis3.domain = domainRange3; + axesSpecs.set(axis3.id, axis3); + + const expectedMergedMap = new Map(); + expectedMergedMap.set(groupId, { min: 2, max: 9 }); + + const mergedDomainsByGroupId = mergeDomainsByGroupId(axesSpecs, 0); + expect(mergedDomainsByGroupId).toEqual(expectedMergedMap); + }); + test('should throw on invalid domain', () => { const domainRange1 = { min: 9, diff --git a/src/lib/axes/axis_utils.ts b/src/lib/axes/axis_utils.ts index de150c968b..c18b06dd56 100644 --- a/src/lib/axes/axis_utils.ts +++ b/src/lib/axes/axis_utils.ts @@ -650,19 +650,19 @@ export function mergeDomainsByGroupId( const prevGroupDomain = domainsByGroupId.get(groupId); if (prevGroupDomain) { - let max; - let min; - const prevMin = prevGroupDomain.min; const prevMax = prevGroupDomain.max; + let max = prevMax; + let min = prevMin; + if ((domain.min != null && domain.max != null)) { - min = prevMin ? Math.min(domain.min, prevMin) : domain.min; - max = prevMax ? Math.max(domain.max, prevMax) : domain.max; + min = prevMin != null ? Math.min(domain.min, prevMin) : domain.min; + max = prevMax != null ? Math.max(domain.max, prevMax) : domain.max; } else if (domain.min != null) { - min = prevMin ? Math.min(domain.min, prevMin) : domain.min; + min = prevMin != null ? Math.min(domain.min, prevMin) : domain.min; } else if (domain.max != null) { - max = prevMax ? Math.max(domain.max, prevMax) : domain.max; + max = prevMax != null ? Math.max(domain.max, prevMax) : domain.max; } const mergedDomain = { From 1940d129f3cdf50a6616165928d75097eed59eb7 Mon Sep 17 00:00:00 2001 From: Emma Cunningham Date: Fri, 22 Mar 2019 12:52:50 -0700 Subject: [PATCH 6/8] test(x_domain): update tests for partial domain --- src/lib/series/domains/x_domain.test.ts | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/lib/series/domains/x_domain.test.ts b/src/lib/series/domains/x_domain.test.ts index 1fc99fac20..0792727568 100644 --- a/src/lib/series/domains/x_domain.test.ts +++ b/src/lib/series/domains/x_domain.test.ts @@ -634,7 +634,7 @@ describe('X Domain', () => { const minInterval = findMinInterval([]); expect(minInterval).toBe(0); }); - test('should account for custom domain when merging a linear domain', () => { + test('should account for custom domain when merging a linear domain: complete bounded domain', () => { const xValues = new Set([1, 2, 3, 4, 5]); const xDomain = { min: 0, max: 3 }; const specs: Array> = [ @@ -659,6 +659,28 @@ describe('X Domain', () => { expect(attemptToMerge).toThrowError('custom xDomain is invalid, min is greater than max'); }); + test('should account for custom domain when merging a linear domain: lower bounded domain', () => { + const xValues = new Set([1, 2, 3, 4, 5]); + const xDomain = { min: 0 }; + const specs: Array> = [ + { seriesType: 'line', xScaleType: ScaleType.Linear }, + ]; + + const mergedDomain = mergeXDomain(specs, xValues, xDomain); + expect(mergedDomain.domain).toEqual([0, 5]); + }); + + test('should account for custom domain when merging a linear domain: upper bounded domain', () => { + const xValues = new Set([1, 2, 3, 4, 5]); + const xDomain = { max: 3 }; + const specs: Array> = [ + { seriesType: 'line', xScaleType: ScaleType.Linear }, + ]; + + const mergedDomain = mergeXDomain(specs, xValues, xDomain); + expect(mergedDomain.domain).toEqual([1, 3]); + }); + test('should account for custom domain when merging an ordinal domain', () => { const xValues = new Set(['a', 'b', 'c', 'd']); const xDomain = ['a', 'b', 'c']; From fbbe316058c9020f69a16b1c6b5b53e933a53986 Mon Sep 17 00:00:00 2001 From: Emma Cunningham Date: Fri, 22 Mar 2019 16:51:04 -0700 Subject: [PATCH 7/8] refactor(domain): add custom guards for DomainRange --- src/lib/axes/axis_utils.test.ts | 14 ++++ src/lib/axes/axis_utils.ts | 53 ++++++++++---- src/lib/series/domains/x_domain.ts | 18 +++-- src/lib/series/domains/y_domain.test.ts | 94 ++++++++++++++++++++++++- src/lib/series/domains/y_domain.ts | 23 +++--- src/state/chart_state.ts | 4 +- src/state/utils.ts | 6 +- 7 files changed, 175 insertions(+), 37 deletions(-) diff --git a/src/lib/axes/axis_utils.test.ts b/src/lib/axes/axis_utils.test.ts index dcbebd5351..38bb9c7220 100644 --- a/src/lib/axes/axis_utils.test.ts +++ b/src/lib/axes/axis_utils.test.ts @@ -21,6 +21,7 @@ import { getVerticalAxisGridLineProps, getVerticalAxisTickLineProps, getVisibleTicks, + isBounded, isHorizontal, isVertical, isYDomain, @@ -1180,4 +1181,17 @@ describe('Axis computational utils', () => { expect(attemptToMerge).toThrowError(expectedError); }); + + test('should determine that a domain has at least one bound', () => { + const lowerBounded = { + min: 0, + }; + + const upperBounded = { + max: 0, + }; + + expect(isBounded(lowerBounded)).toBe(true); + expect(isBounded(upperBounded)).toBe(true); + }); }); diff --git a/src/lib/axes/axis_utils.ts b/src/lib/axes/axis_utils.ts index c18b06dd56..3d9154c5fb 100644 --- a/src/lib/axes/axis_utils.ts +++ b/src/lib/axes/axis_utils.ts @@ -1,7 +1,16 @@ import { XDomain } from '../series/domains/x_domain'; import { YDomain } from '../series/domains/y_domain'; import { computeXScale, computeYScales } from '../series/scales'; -import { AxisSpec, CompleteBoundedDomain, Position, Rotation, TickFormatter } from '../series/specs'; +import { + AxisSpec, + CompleteBoundedDomain, + DomainRange, + LowerBoundedDomain, + Position, + Rotation, + TickFormatter, + UpperBoundedDomain, +} from '../series/specs'; import { AxisConfig, Theme } from '../themes/theme'; import { Dimensions, Margins } from '../utils/dimensions'; import { Domain } from '../utils/domain'; @@ -620,16 +629,30 @@ export function isHorizontal(position: Position) { return !isVertical(position); } +export function isLowerBound(domain: Partial): domain is LowerBoundedDomain { + return domain.min != null; +} + +export function isUpperBound(domain: Partial): domain is UpperBoundedDomain { + return domain.max != null; +} + +export function isCompleteBound(domain: Partial): domain is CompleteBoundedDomain { + return domain.max != null && domain.min != null; +} + +export function isBounded(domain: Partial): domain is DomainRange { + return domain.max != null || domain.min != null; +} + export function mergeDomainsByGroupId( axesSpecs: Map, chartRotation: Rotation, -): Map> { - const domainsByGroupId = new Map>(); +): Map { + const domainsByGroupId = new Map(); axesSpecs.forEach((spec: AxisSpec, id: AxisId) => { - const { groupId } = spec; - - const domain = spec.domain as Partial; + const { groupId, domain } = spec; if (!domain) { return; @@ -642,7 +665,7 @@ export function mergeDomainsByGroupId( throw new Error(errorMessage); } - if ((domain.min != null && domain.max != null) && domain.min > domain.max) { + if (isCompleteBound(domain) && domain.min > domain.max) { const errorMessage = `[Axis ${id}]: custom domain is invalid, min is greater than max`; throw new Error(errorMessage); } @@ -650,18 +673,20 @@ export function mergeDomainsByGroupId( const prevGroupDomain = domainsByGroupId.get(groupId); if (prevGroupDomain) { - const prevMin = prevGroupDomain.min; - const prevMax = prevGroupDomain.max; + const prevDomain = prevGroupDomain as DomainRange; + + const prevMin = (isCompleteBound(prevDomain) || isLowerBound(prevDomain)) ? prevDomain.min : undefined; + const prevMax = (isCompleteBound(prevDomain) || isUpperBound(prevDomain)) ? prevDomain.max : undefined; let max = prevMax; let min = prevMin; - if ((domain.min != null && domain.max != null)) { + if (isCompleteBound(domain)) { min = prevMin != null ? Math.min(domain.min, prevMin) : domain.min; max = prevMax != null ? Math.max(domain.max, prevMax) : domain.max; - } else if (domain.min != null) { + } else if (isLowerBound(domain)) { min = prevMin != null ? Math.min(domain.min, prevMin) : domain.min; - } else if (domain.max != null) { + } else if (isUpperBound(domain)) { max = prevMax != null ? Math.max(domain.max, prevMax) : domain.max; } @@ -670,7 +695,9 @@ export function mergeDomainsByGroupId( max, }; - domainsByGroupId.set(groupId, mergedDomain); + if (isBounded(mergedDomain)) { + domainsByGroupId.set(groupId, mergedDomain); + } } else { domainsByGroupId.set(groupId, domain); } diff --git a/src/lib/series/domains/x_domain.ts b/src/lib/series/domains/x_domain.ts index 83537d60a9..4a85eac467 100644 --- a/src/lib/series/domains/x_domain.ts +++ b/src/lib/series/domains/x_domain.ts @@ -1,7 +1,8 @@ +import { isCompleteBound, isLowerBound, isUpperBound } from '../../axes/axis_utils'; import { compareByValueAsc, identity } from '../../utils/commons'; import { computeContinuousDataDomain, computeOrdinalDataDomain, Domain } from '../../utils/domain'; import { ScaleType } from '../../utils/scales/scales'; -import { BasicSeriesSpec, CompleteBoundedDomain } from '../specs'; +import { BasicSeriesSpec, DomainRange } from '../specs'; import { BaseDomain } from './domain'; export type XDomain = BaseDomain & { @@ -18,7 +19,7 @@ export type XDomain = BaseDomain & { export function mergeXDomain( specs: Array>, xValues: Set, - xDomain?: Partial | Domain, + xDomain?: DomainRange | Domain, ): XDomain { const mainXScaleType = convertXScaleTypes(specs); if (!mainXScaleType) { @@ -42,14 +43,19 @@ export function mergeXDomain( seriesXComputedDomains = computeContinuousDataDomain(values, identity, true); if (xDomain) { if (!Array.isArray(xDomain)) { - if ((xDomain.min != null && xDomain.max != null) && xDomain.min > xDomain.max) { + if (isCompleteBound(xDomain) && xDomain.min > xDomain.max) { throw new Error('custom xDomain is invalid, min is greater than max'); } const [computedDomainMin, computedDomainMax] = seriesXComputedDomains; - const domainMin = xDomain.min == null ? computedDomainMin : xDomain.min; - const domainMax = xDomain.max == null ? computedDomainMax : xDomain.max; - seriesXComputedDomains = [domainMin, domainMax]; + + if (isCompleteBound(xDomain)) { + seriesXComputedDomains = [xDomain.min, xDomain.max]; + } else if (isLowerBound(xDomain)) { + seriesXComputedDomains = [xDomain.min, computedDomainMax]; + } else if (isUpperBound(xDomain)) { + seriesXComputedDomains = [computedDomainMin, xDomain.max]; + } } else { throw new Error('xDomain for continuous scale should be a DomainRange object, not an array'); } diff --git a/src/lib/series/domains/y_domain.test.ts b/src/lib/series/domains/y_domain.test.ts index 49fde02a00..334feaa846 100644 --- a/src/lib/series/domains/y_domain.test.ts +++ b/src/lib/series/domains/y_domain.test.ts @@ -469,7 +469,7 @@ describe('Y Domain', () => { const rawDataSeries = getDataSeriesOnGroup(specDataSeries, specs); expect(rawDataSeries).toEqual([]); }); - test('Should merge Y domain accounting for custom domain limits', () => { + test('Should merge Y domain accounting for custom domain limits: complete bounded domain', () => { const groupId = getGroupId('a'); const dataSeries: RawDataSeries[] = [ @@ -515,4 +515,96 @@ describe('Y Domain', () => { }, ]); }); + test('Should merge Y domain accounting for custom domain limits: partial lower bounded domain', () => { + const groupId = getGroupId('a'); + + const dataSeries: RawDataSeries[] = [ + { + specId: getSpecId('a'), + key: [''], + seriesColorKey: '', + data: [{ x: 1, y: 2 }, { x: 2, y: 2 }, { x: 3, y: 2 }, { x: 4, y: 5 }], + }, + { + specId: getSpecId('a'), + key: [''], + seriesColorKey: '', + data: [{ x: 1, y: 2 }, { x: 4, y: 7 }], + }, + ]; + const specDataSeries = new Map(); + specDataSeries.set(getSpecId('a'), dataSeries); + const domainsByGroupId = new Map(); + domainsByGroupId.set(groupId, { min: 0 }); + + const mergedDomain = mergeYDomain( + specDataSeries, + [ + { + seriesType: 'area', + yScaleType: ScaleType.Linear, + groupId, + id: getSpecId('a'), + stackAccessors: ['a'], + yScaleToDataExtent: true, + }, + ], + domainsByGroupId, + ); + expect(mergedDomain).toEqual([ + { + type: 'yDomain', + groupId, + domain: [0, 12], + scaleType: ScaleType.Linear, + isBandScale: false, + }, + ]); + }); + test('Should merge Y domain accounting for custom domain limits: partial upper bounded domain', () => { + const groupId = getGroupId('a'); + + const dataSeries: RawDataSeries[] = [ + { + specId: getSpecId('a'), + key: [''], + seriesColorKey: '', + data: [{ x: 1, y: 2 }, { x: 2, y: 2 }, { x: 3, y: 2 }, { x: 4, y: 5 }], + }, + { + specId: getSpecId('a'), + key: [''], + seriesColorKey: '', + data: [{ x: 1, y: 2 }, { x: 4, y: 7 }], + }, + ]; + const specDataSeries = new Map(); + specDataSeries.set(getSpecId('a'), dataSeries); + const domainsByGroupId = new Map(); + domainsByGroupId.set(groupId, { max: 20 }); + + const mergedDomain = mergeYDomain( + specDataSeries, + [ + { + seriesType: 'area', + yScaleType: ScaleType.Linear, + groupId, + id: getSpecId('a'), + stackAccessors: ['a'], + yScaleToDataExtent: true, + }, + ], + domainsByGroupId, + ); + expect(mergedDomain).toEqual([ + { + type: 'yDomain', + groupId, + domain: [2, 20], + scaleType: ScaleType.Linear, + isBandScale: false, + }, + ]); + }); }); diff --git a/src/lib/series/domains/y_domain.ts b/src/lib/series/domains/y_domain.ts index dc64b56604..1497c774c2 100644 --- a/src/lib/series/domains/y_domain.ts +++ b/src/lib/series/domains/y_domain.ts @@ -1,10 +1,11 @@ import { sum } from 'd3-array'; +import { isCompleteBound, isLowerBound, isUpperBound } from '../../axes/axis_utils'; import { identity } from '../../utils/commons'; import { computeContinuousDataDomain } from '../../utils/domain'; import { GroupId, SpecId } from '../../utils/ids'; import { ScaleContinuousType, ScaleType } from '../../utils/scales/scales'; import { RawDataSeries } from '../series'; -import { BasicSeriesSpec, CompleteBoundedDomain } from '../specs'; +import { BasicSeriesSpec, DomainRange } from '../specs'; import { BaseDomain } from './domain'; export type YDomain = BaseDomain & { @@ -27,7 +28,7 @@ export type YBasicSeriesSpec = Pick< export function mergeYDomain( dataSeries: Map, specs: YBasicSeriesSpec[], - domainsByGroupId: Map>, + domainsByGroupId: Map, ): YDomain[] { // group specs by group ids const specsByGroupIds = splitSpecsByGroupId(specs); @@ -63,21 +64,19 @@ export function mergeYDomain( isStackedScaleToExtent || isNonStackedScaleToExtent, ); - let domainMin = groupDomain[0]; - let domainMax = groupDomain[1]; + const [computedDomainMin, computedDomainMax] = groupDomain; + let domain = groupDomain; const customDomain = domainsByGroupId.get(groupId); - if (customDomain && customDomain.min != null) { - domainMin = customDomain.min; + if (customDomain && isCompleteBound(customDomain)) { + domain = [customDomain.min, customDomain.max]; + } else if (customDomain && isLowerBound(customDomain)) { + domain = [customDomain.min, computedDomainMax]; + } else if (customDomain && isUpperBound(customDomain)) { + domain = [computedDomainMin, customDomain.max]; } - if (customDomain && customDomain.max != null) { - domainMax = customDomain.max; - } - - const domain = [domainMin, domainMax]; - return { type: 'yDomain', isBandScale: false, diff --git a/src/state/chart_state.ts b/src/state/chart_state.ts index 775c29f04a..5c5f8d8f63 100644 --- a/src/state/chart_state.ts +++ b/src/state/chart_state.ts @@ -32,7 +32,7 @@ import { AxisSpec, BarSeriesSpec, BasicSeriesSpec, - CompleteBoundedDomain, + DomainRange, LineSeriesSpec, Position, Rendering, @@ -139,7 +139,7 @@ export class ChartStore { seriesDomainsAndData?: SeriesDomainsAndData; // computed xScale?: Scale; yScales?: Map; - xDomain?: Domain | Partial; + xDomain?: Domain | DomainRange; legendItems: Map = new Map(); highlightedLegendItemKey: IObservableValue = observable.box(null); diff --git a/src/state/utils.ts b/src/state/utils.ts index 8c59b3ec88..a02eb2e2c8 100644 --- a/src/state/utils.ts +++ b/src/state/utils.ts @@ -27,7 +27,7 @@ import { AreaSeriesSpec, AxisSpec, BasicSeriesSpec, - CompleteBoundedDomain, + DomainRange, LineSeriesSpec, Rotation, } from '../lib/series/specs'; @@ -112,8 +112,8 @@ export function getUpdatedCustomSeriesColors( */ export function computeSeriesDomains( seriesSpecs: Map, - domainsByGroupId: Map>, - customXDomain?: Partial | Domain, + domainsByGroupId: Map, + customXDomain?: DomainRange | Domain, selectedDataSeries?: DataSeriesColorsValues[] | null, ): SeriesDomainsAndData { const { splittedSeries, xValues, seriesColors } = getSplittedSeries( From 9702cb8cce935738c53571b7581265fdd24ee5db Mon Sep 17 00:00:00 2001 From: Emma Cunningham Date: Fri, 22 Mar 2019 17:09:46 -0700 Subject: [PATCH 8/8] test(domain): add tests for invalid domain error throw --- src/lib/axes/axis_utils.ts | 4 +- src/lib/series/domains/x_domain.test.ts | 12 ++++ src/lib/series/domains/x_domain.ts | 16 +++-- src/lib/series/domains/y_domain.test.ts | 84 +++++++++++++++++++++++++ src/lib/series/domains/y_domain.ts | 9 +++ 5 files changed, 119 insertions(+), 6 deletions(-) diff --git a/src/lib/axes/axis_utils.ts b/src/lib/axes/axis_utils.ts index 3d9154c5fb..7f8d95c84d 100644 --- a/src/lib/axes/axis_utils.ts +++ b/src/lib/axes/axis_utils.ts @@ -675,8 +675,8 @@ export function mergeDomainsByGroupId( if (prevGroupDomain) { const prevDomain = prevGroupDomain as DomainRange; - const prevMin = (isCompleteBound(prevDomain) || isLowerBound(prevDomain)) ? prevDomain.min : undefined; - const prevMax = (isCompleteBound(prevDomain) || isUpperBound(prevDomain)) ? prevDomain.max : undefined; + const prevMin = (isLowerBound(prevDomain)) ? prevDomain.min : undefined; + const prevMax = (isUpperBound(prevDomain)) ? prevDomain.max : undefined; let max = prevMax; let min = prevMin; diff --git a/src/lib/series/domains/x_domain.test.ts b/src/lib/series/domains/x_domain.test.ts index 0792727568..10e2efb758 100644 --- a/src/lib/series/domains/x_domain.test.ts +++ b/src/lib/series/domains/x_domain.test.ts @@ -668,6 +668,12 @@ describe('X Domain', () => { const mergedDomain = mergeXDomain(specs, xValues, xDomain); expect(mergedDomain.domain).toEqual([0, 5]); + + const invalidXDomain = { min: 10 }; + const attemptToMerge = () => { + mergeXDomain(specs, xValues, invalidXDomain); + }; + expect(attemptToMerge).toThrowError('custom xDomain is invalid, custom min is greater than computed max'); }); test('should account for custom domain when merging a linear domain: upper bounded domain', () => { @@ -679,6 +685,12 @@ describe('X Domain', () => { const mergedDomain = mergeXDomain(specs, xValues, xDomain); expect(mergedDomain.domain).toEqual([1, 3]); + + const invalidXDomain = { max: -1 }; + const attemptToMerge = () => { + mergeXDomain(specs, xValues, invalidXDomain); + }; + expect(attemptToMerge).toThrowError('custom xDomain is invalid, computed min is greater than custom max'); }); test('should account for custom domain when merging an ordinal domain', () => { diff --git a/src/lib/series/domains/x_domain.ts b/src/lib/series/domains/x_domain.ts index 4a85eac467..fba4d039e1 100644 --- a/src/lib/series/domains/x_domain.ts +++ b/src/lib/series/domains/x_domain.ts @@ -43,17 +43,25 @@ export function mergeXDomain( seriesXComputedDomains = computeContinuousDataDomain(values, identity, true); if (xDomain) { if (!Array.isArray(xDomain)) { - if (isCompleteBound(xDomain) && xDomain.min > xDomain.max) { - throw new Error('custom xDomain is invalid, min is greater than max'); - } - const [computedDomainMin, computedDomainMax] = seriesXComputedDomains; if (isCompleteBound(xDomain)) { + if (xDomain.min > xDomain.max) { + throw new Error('custom xDomain is invalid, min is greater than max'); + } + seriesXComputedDomains = [xDomain.min, xDomain.max]; } else if (isLowerBound(xDomain)) { + if (xDomain.min > computedDomainMax) { + throw new Error('custom xDomain is invalid, custom min is greater than computed max'); + } + seriesXComputedDomains = [xDomain.min, computedDomainMax]; } else if (isUpperBound(xDomain)) { + if (computedDomainMin > xDomain.max) { + throw new Error('custom xDomain is invalid, computed min is greater than custom max'); + } + seriesXComputedDomains = [computedDomainMin, xDomain.max]; } } else { diff --git a/src/lib/series/domains/y_domain.test.ts b/src/lib/series/domains/y_domain.test.ts index 334feaa846..e0601a56e2 100644 --- a/src/lib/series/domains/y_domain.test.ts +++ b/src/lib/series/domains/y_domain.test.ts @@ -561,6 +561,48 @@ describe('Y Domain', () => { }, ]); }); + test('Should not merge Y domain with invalid custom domain limits: partial lower bounded domain', () => { + const groupId = getGroupId('a'); + + const dataSeries: RawDataSeries[] = [ + { + specId: getSpecId('a'), + key: [''], + seriesColorKey: '', + data: [{ x: 1, y: 2 }, { x: 2, y: 2 }, { x: 3, y: 2 }, { x: 4, y: 5 }], + }, + { + specId: getSpecId('a'), + key: [''], + seriesColorKey: '', + data: [{ x: 1, y: 2 }, { x: 4, y: 7 }], + }, + ]; + const specDataSeries = new Map(); + specDataSeries.set(getSpecId('a'), dataSeries); + const domainsByGroupId = new Map(); + domainsByGroupId.set(groupId, { min: 20 }); + + const attemptToMerge = () => { + mergeYDomain( + specDataSeries, + [ + { + seriesType: 'area', + yScaleType: ScaleType.Linear, + groupId, + id: getSpecId('a'), + stackAccessors: ['a'], + yScaleToDataExtent: true, + }, + ], + domainsByGroupId, + ); + }; + + const errorMessage = 'custom yDomain for a is invalid, custom min is greater than computed max'; + expect(attemptToMerge).toThrowError(errorMessage); + }); test('Should merge Y domain accounting for custom domain limits: partial upper bounded domain', () => { const groupId = getGroupId('a'); @@ -607,4 +649,46 @@ describe('Y Domain', () => { }, ]); }); + test('Should not merge Y domain with invalid custom domain limits: partial upper bounded domain', () => { + const groupId = getGroupId('a'); + + const dataSeries: RawDataSeries[] = [ + { + specId: getSpecId('a'), + key: [''], + seriesColorKey: '', + data: [{ x: 1, y: 2 }, { x: 2, y: 2 }, { x: 3, y: 2 }, { x: 4, y: 5 }], + }, + { + specId: getSpecId('a'), + key: [''], + seriesColorKey: '', + data: [{ x: 1, y: 2 }, { x: 4, y: 7 }], + }, + ]; + const specDataSeries = new Map(); + specDataSeries.set(getSpecId('a'), dataSeries); + const domainsByGroupId = new Map(); + domainsByGroupId.set(groupId, { max: -1 }); + + const attemptToMerge = () => { + mergeYDomain( + specDataSeries, + [ + { + seriesType: 'area', + yScaleType: ScaleType.Linear, + groupId, + id: getSpecId('a'), + stackAccessors: ['a'], + yScaleToDataExtent: true, + }, + ], + domainsByGroupId, + ); + }; + + const errorMessage = 'custom yDomain for a is invalid, computed min is greater than custom max'; + expect(attemptToMerge).toThrowError(errorMessage); + }); }); diff --git a/src/lib/series/domains/y_domain.ts b/src/lib/series/domains/y_domain.ts index 1497c774c2..0c2006fad4 100644 --- a/src/lib/series/domains/y_domain.ts +++ b/src/lib/series/domains/y_domain.ts @@ -70,10 +70,19 @@ export function mergeYDomain( const customDomain = domainsByGroupId.get(groupId); if (customDomain && isCompleteBound(customDomain)) { + // Don't need to check min > max because this has been validated on axis domain merge domain = [customDomain.min, customDomain.max]; } else if (customDomain && isLowerBound(customDomain)) { + if (customDomain.min > computedDomainMax) { + throw new Error(`custom yDomain for ${groupId} is invalid, custom min is greater than computed max`); + } + domain = [customDomain.min, computedDomainMax]; } else if (customDomain && isUpperBound(customDomain)) { + if (computedDomainMin > customDomain.max) { + throw new Error(`custom yDomain for ${groupId} is invalid, computed min is greater than custom max`); + } + domain = [computedDomainMin, customDomain.max]; }