Skip to content

Honour axis min/max settings #4522

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

Merged
merged 2 commits into from
Jul 22, 2017
Merged

Honour axis min/max settings #4522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8c8dc3
Honor axis min/max settings
andig Jul 18, 2017
45dde56
Fix typos
andig Jul 18, 2017
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
20 changes: 16 additions & 4 deletions src/helpers/helpers.time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
var moment = require('moment');
moment = typeof(moment) === 'function' ? moment : window.moment;

var helpers = require('./helpers.core');

var interval = {
millisecond: {
size: 1,
Expand Down Expand Up @@ -53,25 +55,35 @@ function generateTicksNiceRange(options, dataRange, niceRange) {
var ticks = [];
if (options.maxTicks) {
var stepSize = options.stepSize;
var startTick = options.min !== undefined ? options.min : niceRange.min;
var startTick = helpers.isNullOrUndef(options.min) ? niceRange.min : options.min;
var majorUnit = options.majorUnit;
var majorUnitStart = majorUnit ? moment(startTick).add(1, majorUnit).startOf(majorUnit) : startTick;
var startRange = majorUnitStart.valueOf() - startTick;
var stepValue = interval[options.unit].size * stepSize;
var startFraction = startRange % stepValue;
var alignedTick = startTick;
if (startFraction && majorUnit && !options.timeOpts.round && !options.timeOpts.isoWeekday) {

// first tick
if (startFraction && majorUnit && !options.timeOpts.round && !options.timeOpts.isoWeekday && helpers.isNullOrUndef(options.min)) {
Copy link
Member

@etimberg etimberg Jul 18, 2017

Choose a reason for hiding this comment

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

I agree that this if is dubious. I'm not sure why isoWeekday is doing things here.

Copy link
Contributor

@benmccann benmccann Jul 18, 2017

Choose a reason for hiding this comment

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

I can't quite tell what the isoWeekday option is supposed to do from the docs. Looking at the code it seems that perhaps that setting isoWeekday makes every tick a Monday. If you're manually setting each tick to a Monday then it'd make sense not to do smart alignment here, but it's quite difficult to tell what this option is. I tried to build a sample to test it out, but the time samples seem to be currently broken on master. After #4507 is merged I imagine the samples will be working again and then we can take a stab at better documenting that feature and seeing if we should remove isoWeekday here

Copy link
Contributor Author

@andig andig Jul 18, 2017

Choose a reason for hiding this comment

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

I'd leave it unchanged for now as removing it breaks this https://github.com/chartjs/Chart.js/blob/master/test/specs/scale.time.tests.js#L331 test. The test itself is odd as isoWeekDay should be a boolean but the tests uses numerical value.

Copy link
Member

@simonbrunel simonbrunel Jul 18, 2017

Choose a reason for hiding this comment

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

My understanding of isoWeekDay is that's a number representing the day (1: Monday - 7: Sunday) that should be considered the first day of the week and I guess that isoWeekDay: true is an alias to 1 (Monday).

Copy link
Contributor Author

@andig andig Jul 18, 2017

Choose a reason for hiding this comment

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

Then the docs are off: https://github.com/chartjs/Chart.js/blob/master/docs/axes/cartesian/time.md shows its boolean.

alignedTick += startFraction - stepValue;
ticks.push(alignedTick);
} else {
ticks.push(startTick);
}

// generate remaining ticks
var cur = moment(alignedTick);
var realMax = options.max || niceRange.max;
var realMax = helpers.isNullOrUndef(options.max) ? niceRange.max : options.max;
while (cur.add(stepSize, options.unit).valueOf() < realMax) {
ticks.push(cur.valueOf());
}
ticks.push(cur.valueOf());

// last tick
if (helpers.isNullOrUndef(options.max)) {
Copy link
Member

@etimberg etimberg Jul 18, 2017

Choose a reason for hiding this comment

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

minor question. could we always just push realMax at the end?

Copy link
Contributor Author

@andig andig Jul 18, 2017

Choose a reason for hiding this comment

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

You can't because cur.valueOf() > niceRange.max in this case, meaning that the last tick will be too early. Begs the question what niceRangeMax really is and where it comes from. Didn't look into this- the current change is minimally invasive.

ticks.push(cur.valueOf());
} else {
ticks.push(realMax);
}
}
return ticks;
}
Expand Down
14 changes: 7 additions & 7 deletions test/specs/scale.time.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,17 @@ describe('Time scale tests', function() {
});
});

it('Should load moment.js as a dependency', function() {
it('should load moment.js as a dependency', function() {
expect(window.moment).not.toBe(undefined);
});

it('Should register the constructor with the scale service', function() {
it('should register the constructor with the scale service', function() {
var Constructor = Chart.scaleService.getScaleConstructor('time');
expect(Constructor).not.toBe(undefined);
expect(typeof Constructor).toBe('function');
});

it('Should have the correct default config', function() {
it('should have the correct default config', function() {
var defaultConfig = Chart.scaleService.getScaleDefaults('time');
expect(defaultConfig).toEqual({
display: true,
Expand Down Expand Up @@ -315,7 +315,7 @@ describe('Time scale tests', function() {
config.time.min = '2014-12-29T04:00:00';

var scale = createScale(mockData, config);
expect(scale.ticks[0].value).toEqual('Dec 28');
expect(scale.ticks[0].value).toEqual('Dec 29');
});

it('should use the max option', function() {
Expand All @@ -324,11 +324,11 @@ describe('Time scale tests', function() {

var scale = createScale(mockData, config);

expect(scale.ticks[scale.ticks.length - 1].value).toEqual('Jan 6');
expect(scale.ticks[scale.ticks.length - 1].value).toEqual('Jan 5');
});
});

it('Should use the isoWeekday option', function() {
it('should use the isoWeekday option', function() {
var mockData = {
labels: [
'2015-01-01T20:00:00', // Thursday
Expand Down Expand Up @@ -420,7 +420,7 @@ describe('Time scale tests', function() {
var step = xScale.ticksAsTimestamps[1] - xScale.ticksAsTimestamps[0];
var stepsAmount = Math.floor((xScale.max - xScale.min) / step);

it('should be bounded by nearest step year starts', function() {
it('should be bounded by nearest step\'s year start and end', function() {
expect(xScale.getValueForPixel(xScale.left)).toBeCloseToTime({
value: moment(xScale.min).startOf('year'),
unit: 'hour',
Expand Down