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

fix: x-scale for linear band charts #384

Merged
merged 5 commits into from
Sep 27, 2019
Merged

fix: x-scale for linear band charts #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d79c80
build(tsconfig): update storybook and playground tsconfig
nickofthyme Sep 13, 2019
f30fd22
fix: x-scale for linear band charts
nickofthyme Sep 21, 2019
3977b7f
Merge branch 'master' into fix/scale-tick-steps
nickofthyme Sep 21, 2019
3b3c5ef
fix: pr comment
nickofthyme Sep 27, 2019
4902a15
test: add test for fix
nickofthyme Sep 27, 2019
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
68 changes: 30 additions & 38 deletions .playground/playgroud.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,50 +1,42 @@
import React, { Fragment } from 'react';
import { Axis, Chart, getAxisId, getSpecId, Position, ScaleType, Settings, BarSeries } from '../src';
import { Axis, Settings, Chart, getAxisId, getSpecId, Position, ScaleType, AreaSeries, CurveType } from '../src';

export class Playground extends React.Component {
render() {
const data = [
{ x: 7.053400039672852, y: 1.019049570549345 },
{ x: 16.8664653595564, y: 1.5285743558240172 },
{ x: 26.67953067943995, y: 0.5095247852746725 },
{ x: 36.4925959993235, y: 0.12998767296647204 },
{ x: 74.95778185805996, y: 0.3718786139686189 },
{ x: 88.40302934524654, y: 0.14487824285108267 },
{ x: 122.9147676270215, y: 0.07890686802154025 },
{ x: 186.28060795710638, y: 0.4344198127360625 },
{ x: 197.79021192408248, y: 0.47910304703632484 },
{ x: 208.22638015747071, y: 0.15180409193531094 },
{ x: 241.16356871580467, y: 0.0778711327650822 },
{ x: 305.3722147500643, y: 0.05038552439310782 },
{ x: 404.60706563679923, y: 0.04950569918337908 },
{ x: 505.60553818596964, y: 0.010256529428346779 },
{ x: 993.0998738606771, y: 0.06490505477669992 },
{ x: 1070.1354763603908, y: 0 },
];
return (
<Fragment>
<div className="chart">
<Chart>
<Settings
showLegend={true}
theme={{
axes: {
gridLineStyle: {
horizontal: {
stroke: 'red',
strokeWidth: 0.5,
opacity: 1,
dash: [0, 0],
},
vertical: {
stroke: 'blue',
strokeWidth: 0.5,
opacity: 1,
dash: [4, 4],
},
},
},
}}
/>
<Axis
id={getAxisId('y')}
position={Position.Left}
domain={{
min: 50,
max: 250,
}}
showGridLines
/>
<Axis showGridLines id={getAxisId('x')} position={Position.Bottom} />
<BarSeries
id={getSpecId('bar')}
yScaleType={ScaleType.Linear}
<Settings />
<Axis id={getAxisId('bottom')} position={Position.Bottom} tickFormat={(d) => d.toFixed(0)} ticks={5} />
<Axis id={getAxisId('left')} position={Position.Left} tickFormat={(d) => d.toFixed(1)} hide={true} />
<AreaSeries
id={getSpecId('aaa')}
name={'aaa'}
xScaleType={ScaleType.Linear}
xAccessor={0}
yAccessors={[1]}
data={[[0, 100], [1, 50], [3, 400], [4, 250], [5, 235]]}
yScaleType={ScaleType.Linear}
xAccessor="x"
yAccessors={['y']}
data={data}
curve={CurveType.CURVE_STEP_AFTER}
/>
</Chart>
</div>
Expand Down
16 changes: 16 additions & 0 deletions src/utils/scales/scale_continuous.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,22 @@ describe('Scale Continuous', () => {
});
});

describe('xScale values with minInterval and bandwidth', () => {
const domain = [7.053400039672852, 1070.1354763603908];

it('should return nice ticks when minInterval & bandwidth are 0', () => {
const scale = new ScaleContinuous(
{
type: ScaleType.Linear,
domain,
range: [0, 100],
},
{ minInterval: 0, bandwidth: 0 },
);
expect(scale.ticks()).toEqual([100, 200, 300, 400, 500, 600, 700, 800, 900, 1000]);
});
});

describe('time ticks', () => {
const timezonesToTest = ['Asia/Tokyo', 'Europe/Berlin', 'UTC', 'America/New_York', 'America/Los_Angeles'];

Expand Down
53 changes: 38 additions & 15 deletions src/utils/scales/scale_continuous.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,29 @@
import { bisectLeft } from 'd3-array';
import { scaleLinear, scaleLog, scaleSqrt, scaleUtc } from 'd3-scale';
import {
scaleLinear,
scaleLog,
scaleSqrt,
scaleUtc,
ScaleLinear,
ScaleLogarithmic,
ScalePower,
ScaleTime,
} from 'd3-scale';
import { DateTime } from 'luxon';

import { clamp, mergePartial } from '../commons';
import { ScaleContinuousType, ScaleType, Scale } from './scales';

/**
* d3 scales excluding time scale
*/
type D3ScaleNonTime = ScaleLinear<any, any> | ScaleLogarithmic<any, any> | ScalePower<any, any>;

/**
* All possible d3 scales
*/
type D3Scale = D3ScaleNonTime | ScaleTime<any, any>;

const SCALES = {
[ScaleType.Linear]: scaleLinear,
[ScaleType.Log]: scaleLog,
Expand Down Expand Up @@ -129,7 +149,7 @@ export class ScaleContinuous implements Scale {
readonly timeZone: string;
readonly barsPadding: number;
readonly isSingleValueHistogram: boolean;
private readonly d3Scale: any;
private readonly d3Scale: D3Scale;

constructor(scaleData: ScaleData, options?: Partial<ScaleOptions>) {
const { type, domain, range } = scaleData;
Expand All @@ -144,13 +164,10 @@ export class ScaleContinuous implements Scale {
} = mergePartial(defaultScaleOptions, options);

this.d3Scale = SCALES[type]();
if (type === ScaleType.Log) {
this.domain = limitLogScaleDomain(domain);
this.d3Scale.domain(this.domain);
} else {
this.domain = domain;
this.d3Scale.domain(domain);
}
const cleanDomain = type === ScaleType.Log ? limitLogScaleDomain(domain) : domain;
this.domain = cleanDomain;
this.d3Scale.domain(cleanDomain);

const safeBarPadding = clamp(barsPadding, 0, 1);
this.barsPadding = safeBarPadding;
this.bandwidth = bandwidth * (1 - safeBarPadding);
Expand Down Expand Up @@ -182,13 +199,18 @@ export class ScaleContinuous implements Scale {
return currentDateTime.minus({ minutes: currentOffset }).toMillis();
});
} else {
if (this.minInterval > 0) {
/**
* This case is for the xScale (minInterval is > 0) when we want to show bars (bandwidth > 0)
*
* We want to avoid displaying inner ticks between bars in a bar chart when using linear x scale
*/
if (minInterval > 0 && bandwidth > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding a comment before this line specifying that:

  • this case happens only on the xScale (minInterval is > 0 only in that case) and when we want to show bars (bandwidth > 0)
  • the reason is: we want to avoid displaying inner ticks between bars in a bar chart when using linear x scale: this can be misinterpreted because the bar width can be interpreted as an interval along that scale

Screenshot 2019-09-27 at 10 52 52

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

3b3c5ef

const intervalCount = Math.floor((this.domain[1] - this.domain[0]) / this.minInterval);
this.tickValues = new Array(intervalCount + 1).fill(0).map((d, i) => {
this.tickValues = new Array(intervalCount + 1).fill(0).map((_, i) => {
return this.domain[0] + i * this.minInterval;
});
} else {
this.tickValues = this.d3Scale.ticks(ticks);
this.tickValues = (this.d3Scale as D3ScaleNonTime).ticks(ticks);
}
}
}
Expand All @@ -205,12 +227,13 @@ export class ScaleContinuous implements Scale {
ticks() {
return this.tickValues;
}
invert(value: number) {
invert(value: number): number {
let invertedValue = this.d3Scale.invert(value);
if (this.type === ScaleType.Time) {
invertedValue = DateTime.fromJSDate(invertedValue).toMillis();
invertedValue = DateTime.fromJSDate(invertedValue as Date).toMillis();
}
return invertedValue;

return invertedValue as number;
}
invertWithStep(
value: number,
Expand Down