Skip to content

Commit

Permalink
fix(histogram): fix overflowing annotation with single value (#343)
Browse files Browse the repository at this point in the history
This commit fix an issue where a rect annotation with x0 defined in a single value histogram, is
drawn outside of the chart.

BREAKING CHANGE: The current coordinate configuration of a rect annotation were inverted. This commit now reverse them: a rect coordinate with only the x0 value will cover from the x0 value to the end of the domain, a rect coordinate with only the x1 value will cover the interval from the beginning of the domain till the x1 value.

fix #342, fix #341
  • Loading branch information
markov00 authored Aug 26, 2019
1 parent d47de40 commit 2268f04
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 24 deletions.
10 changes: 5 additions & 5 deletions src/chart_types/xy_chart/annotations/annotation_utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1392,14 +1392,14 @@ describe('annotation utils', () => {
const dimensions = computeRectAnnotationDimensions(annotationRectangle, yScales, xScale, true, 0);

const [dims1, dims2, dims3, dims4] = dimensions;
expect(dims1.rect.x).toBe(0);
expect(dims1.rect.x).toBe(10);
expect(dims1.rect.y).toBe(0);
expect(dims1.rect.width).toBe(10);
expect(dims1.rect.width).toBeCloseTo(100);
expect(dims1.rect.height).toBe(100);

expect(dims2.rect.x).toBe(10);
expect(dims2.rect.x).toBe(0);
expect(dims2.rect.y).toBe(0);
expect(dims2.rect.width).toBeCloseTo(100);
expect(dims2.rect.width).toBe(10);
expect(dims2.rect.height).toBe(100);

expect(dims3.rect.x).toBe(0);
Expand Down Expand Up @@ -1433,8 +1433,8 @@ describe('annotation utils', () => {
const dimensions = computeRectAnnotationDimensions(annotationRectangle, yScales, xScale, false, 0);

const expectedDimensions = [
{ rect: { x: 0, y: 0, width: 10, height: 100 } },
{ rect: { x: 10, y: 0, width: 90, height: 100 } },
{ rect: { x: 0, y: 0, width: 10, height: 100 } },
{ rect: { x: 0, y: 0, width: 100, height: 10 } },
{ rect: { x: 0, y: 10, width: 100, height: 90 } },
];
Expand Down
9 changes: 4 additions & 5 deletions src/chart_types/xy_chart/annotations/annotation_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,6 @@ export function computeRectAnnotationDimensions(
const yDomain = yScale.domain;
const lastX = xDomain[xDomain.length - 1];
const xMinInterval = xScale.minInterval;

const rectsProps: AnnotationRectProps[] = [];

dataValues.forEach((dataValue: RectAnnotationDatum) => {
Expand All @@ -375,15 +374,15 @@ export function computeRectAnnotationDimensions(
return;
}

if (x0 == null) {
if (x1 == null) {
// if x1 is defined, we want the rect to draw to the end of the scale
// if we're in histogram mode, extend domain end by min interval
x0 = enableHistogramMode ? lastX + xMinInterval : lastX;
x1 = enableHistogramMode && !xScale.isSingleValue() ? lastX + xMinInterval : lastX;
}

if (x1 == null) {
if (x0 == null) {
// if x0 is defined, we want the rect to draw to the start of the scale
x1 = xDomain[0];
x0 = xDomain[0];
}

if (y0 == null) {
Expand Down
10 changes: 9 additions & 1 deletion src/chart_types/xy_chart/utils/scales.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,15 @@ export function computeXScale(options: XScaleOptions): Scale {
domain: adjustedDomain,
range: [start, end],
},
{ bandwidth: bandwidth / totalBarsInCluster, minInterval, timeZone, totalBarsInCluster, barsPadding, ticks },
{
bandwidth: bandwidth / totalBarsInCluster,
minInterval,
timeZone,
totalBarsInCluster,
barsPadding,
ticks,
isSingleValueHistogram,
},
);

return scale;
Expand Down
39 changes: 26 additions & 13 deletions src/utils/scales/scale_continuous.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,18 @@ interface ScaleOptions {
* @default 10
*/
ticks: number;
/** true if the scale was adjusted to fit one single value histogram */
isSingleValueHistogram: boolean;
}

const defaultScaleOptions: ScaleOptions = {
bandwidth: 0,
minInterval: 0,
timeZone: 'utc',
totalBarsInCluster: 1,
barsPadding: 0,
ticks: 10,
isSingleValueHistogram: false,
};
export class ScaleContinuous implements Scale {
readonly bandwidth: number;
readonly totalBarsInCluster: number;
Expand All @@ -118,22 +128,21 @@ export class ScaleContinuous implements Scale {
readonly tickValues: number[];
readonly timeZone: string;
readonly barsPadding: number;
readonly isSingleValueHistogram: boolean;
private readonly d3Scale: any;

constructor(scaleData: ScaleData, options?: Partial<ScaleOptions>) {
const { type, domain, range } = scaleData;
const scaleOptions: ScaleOptions = mergePartial(
{
bandwidth: 0,
minInterval: 0,
timeZone: 'utc',
totalBarsInCluster: 1,
barsPadding: 0,
ticks: 10,
},
options,
);
const { bandwidth, minInterval, timeZone, totalBarsInCluster, barsPadding, ticks } = scaleOptions;
const {
bandwidth,
minInterval,
timeZone,
totalBarsInCluster,
barsPadding,
ticks,
isSingleValueHistogram,
} = mergePartial(defaultScaleOptions, options);

this.d3Scale = SCALES[type]();
if (type === ScaleType.Log) {
this.domain = limitLogScaleDomain(domain);
Expand All @@ -154,6 +163,7 @@ export class ScaleContinuous implements Scale {
this.isInverted = this.domain[0] > this.domain[1];
this.timeZone = timeZone;
this.totalBarsInCluster = totalBarsInCluster;
this.isSingleValueHistogram = isSingleValueHistogram;
if (type === ScaleType.Time) {
const startDomain = DateTime.fromMillis(this.domain[0], { zone: this.timeZone });
const endDomain = DateTime.fromMillis(this.domain[1], { zone: this.timeZone });
Expand Down Expand Up @@ -242,6 +252,9 @@ export class ScaleContinuous implements Scale {
};
}
isSingleValue() {
if (this.isSingleValueHistogram) {
return true;
}
if (this.domain.length < 2) {
return true;
}
Expand Down

0 comments on commit 2268f04

Please sign in to comment.