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

Jump to bottom

Change scale.ticks back to an array of strings #4573

Merged
merged 2 commits into from
Jul 29, 2017
Merged

Change scale.ticks back to an array of strings #4573

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39091d0
Change `scale.ticks` back to an array of strings
simonbrunel Jul 27, 2017
778e49f
Better explanation of the backward compatibility
simonbrunel Jul 28, 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
2 changes: 1 addition & 1 deletion src/controllers/controller.bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ module.exports = function(Chart) {
var options = scale.options;
var stackCount = me.getStackCount();
var fullSize = scale.isHorizontal() ? scale.width : scale.height;
var tickSize = fullSize / scale.ticks.length;
var tickSize = fullSize / scale.getTicks().length;
var categorySize = tickSize * options.categoryPercentage;
var fullBarSize = categorySize / stackCount;
var barSize = fullBarSize * options.barPercentage;
Expand Down
86 changes: 65 additions & 21 deletions src/core/core.scale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@ defaults._set('scale', {
}
});

function labelsFromTicks(ticks) {
var labels = [];
var i, ilen;

for (i = 0, ilen = ticks.length; i < ilen; ++i) {
labels.push(ticks[i].label);
}

return labels;
}

module.exports = function(Chart) {

function computeTextSize(context, tick, font) {
Expand Down Expand Up @@ -103,6 +114,14 @@ module.exports = function(Chart) {
};
},

/**
* Returns the scale tick objects ({label, major})
* @since 2.7
*/
getTicks: function() {
return this._ticks;
},

// These methods are ordered by lifecyle. Utilities then follow.
// Any function defined here is inherited by all scale types.
// Any function can be extended by the scale type
Expand Down Expand Up @@ -135,6 +154,7 @@ module.exports = function(Chart) {
},
update: function(maxWidth, maxHeight, margins) {
var me = this;
var i, ilen, labels, label, ticks, tick;

// Update Lifecycle - Probably don't want to ever extend or overwrite this function ;)
me.beforeUpdate();
Expand Down Expand Up @@ -162,13 +182,36 @@ module.exports = function(Chart) {

// Ticks
me.beforeBuildTicks();
me.buildTicks();
ticks = me.buildTicks() || [];
Copy link
Member

Choose a reason for hiding this comment

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

is changing this to return breaking?

Copy link
Member Author

@simonbrunel simonbrunel Jul 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I hope not: if buildTicks doesn't return anything, that means it's the old implementation that directly store ticks in this.ticks. It also means this same object will be used by the old implementation of convertTicksToLabels (the base one still use me.ticks).

Here, ticks is supposed to be an array of objects: if nothing is returned, it's re-synchronized with labels bellow (and so me.ticks for old impl.).

me.afterBuildTicks();

me.beforeTickToLabelConversion();
me.convertTicksToLabels();
labels = me.convertTicksToLabels(ticks) || me.ticks;
Copy link
Member

Choose a reason for hiding this comment

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

is || me.ticks correct? Should it be || ticks ?

Copy link
Member Author

@simonbrunel simonbrunel Jul 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it's me.ticks in case of a sub class that convertTicksToLabels in place and so modify me.ticks directly without returning it. I know it's pretty confusing, I will add a comment.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense 👍

me.afterTickToLabelConversion();

me.ticks = labels; // BACKWARD COMPATIBILITY

// IMPORTANT: from this point, we consider that this.ticks will NEVER change!
// Internal ticks are now stored as object in the PRIVATE this._ticks member and
// must not be accessed directly from outside this class. this.ticks is around
// for long time and hasn't been marked private, so we can't change its structure
// without unexpected breaking changes. If you need to access the scale ticks,
// use scale.getTicks() instead.
for (i = 0, ilen = labels.length; i < ilen; ++i) {
label = labels[i];
tick = ticks[i];
if (!tick) {
ticks.push(tick = {
label: label,
major: false
});
} else {
tick.label = label;
}
}

me._ticks = ticks;

// Tick Rotation
me.beforeCalculateTickRotation();
me.calculateTickRotation();
Expand Down Expand Up @@ -258,6 +301,7 @@ module.exports = function(Chart) {
var me = this;
var context = me.ctx;
var tickOpts = me.options.ticks;
var labels = labelsFromTicks(me._ticks);

// Get the width of each grid by calculating the difference
// between x offsets between 0 and 1.
Expand All @@ -266,8 +310,8 @@ module.exports = function(Chart) {

var labelRotation = tickOpts.minRotation || 0;

if (me.options.display && me.isHorizontal()) {
var originalLabelWidth = helpers.longestText(context, tickFont.font, me.ticks, me.longestTextCache);
if (labels.length && me.options.display && me.isHorizontal()) {
var originalLabelWidth = helpers.longestText(context, tickFont.font, labels, me.longestTextCache);
var labelWidth = originalLabelWidth;
var cosRotation;
var sinRotation;
Expand Down Expand Up @@ -311,6 +355,8 @@ module.exports = function(Chart) {
height: 0
};

var labels = labelsFromTicks(me._ticks);

var opts = me.options;
var tickOpts = opts.ticks;
var scaleLabelOpts = opts.scaleLabel;
Expand Down Expand Up @@ -348,8 +394,8 @@ module.exports = function(Chart) {

// Don't bother fitting the ticks if we are not showing them
if (tickOpts.display && display) {
var largestTextWidth = helpers.longestText(me.ctx, tickFont.font, me.ticks, me.longestTextCache);
var tallestLabelHeightInLines = helpers.numberOfLabelLines(me.ticks);
var largestTextWidth = helpers.longestText(me.ctx, tickFont.font, labels, me.longestTextCache);
var tallestLabelHeightInLines = helpers.numberOfLabelLines(labels);
var lineSpace = tickFont.size * 0.5;
var tickPadding = me.options.ticks.padding;

Expand All @@ -369,11 +415,8 @@ module.exports = function(Chart) {
minSize.height = Math.min(me.maxHeight, minSize.height + labelHeight + tickPadding);
me.ctx.font = tickFont.font;

var firstTick = me.ticks[0];
var firstLabelWidth = computeTextSize(me.ctx, firstTick, tickFont.font);

var lastTick = me.ticks[me.ticks.length - 1];
var lastLabelWidth = computeTextSize(me.ctx, lastTick, tickFont.font);
var firstLabelWidth = computeTextSize(me.ctx, labels[0], tickFont.font);
var lastLabelWidth = computeTextSize(me.ctx, labels[labels.length - 1], tickFont.font);

// Ensure that our ticks are always inside the canvas. When rotated, ticks are right aligned which means that the right padding is dominated
// by the font height
Expand Down Expand Up @@ -471,7 +514,7 @@ module.exports = function(Chart) {
var me = this;
if (me.isHorizontal()) {
var innerWidth = me.width - (me.paddingLeft + me.paddingRight);
var tickWidth = innerWidth / Math.max((me.ticks.length - ((me.options.gridLines.offsetGridLines) ? 0 : 1)), 1);
var tickWidth = innerWidth / Math.max((me._ticks.length - ((me.options.gridLines.offsetGridLines) ? 0 : 1)), 1);
var pixel = (tickWidth * index) + me.paddingLeft;

if (includeOffset) {
Expand All @@ -483,7 +526,7 @@ module.exports = function(Chart) {
return finalVal;
}
var innerHeight = me.height - (me.paddingTop + me.paddingBottom);
return me.top + (index * (innerHeight / (me.ticks.length - 1)));
return me.top + (index * (innerHeight / (me._ticks.length - 1)));
},

// Utility for getting the pixel location of a percentage of scale
Expand Down Expand Up @@ -555,20 +598,21 @@ module.exports = function(Chart) {
var labelRotationRadians = helpers.toRadians(me.labelRotation);
var cosRotation = Math.cos(labelRotationRadians);
var longestRotatedLabel = me.longestLabelWidth * cosRotation;
var tickCount = me._ticks.length;

var itemsToDraw = [];

if (isHorizontal) {
skipRatio = false;

if ((longestRotatedLabel + optionTicks.autoSkipPadding) * me.ticks.length > (me.width - (me.paddingLeft + me.paddingRight))) {
skipRatio = 1 + Math.floor(((longestRotatedLabel + optionTicks.autoSkipPadding) * me.ticks.length) / (me.width - (me.paddingLeft + me.paddingRight)));
if ((longestRotatedLabel + optionTicks.autoSkipPadding) * tickCount > (me.width - (me.paddingLeft + me.paddingRight))) {
skipRatio = 1 + Math.floor(((longestRotatedLabel + optionTicks.autoSkipPadding) * tickCount) / (me.width - (me.paddingLeft + me.paddingRight)));
}

// if they defined a max number of optionTicks,
// increase skipRatio until that number is met
if (maxTicks && me.ticks.length > maxTicks) {
while (!skipRatio || me.ticks.length / (skipRatio || 1) > maxTicks) {
if (maxTicks && tickCount > maxTicks) {
while (!skipRatio || tickCount / (skipRatio || 1) > maxTicks) {
if (!skipRatio) {
skipRatio = 1;
}
Expand All @@ -587,17 +631,17 @@ module.exports = function(Chart) {
var yTickStart = options.position === 'bottom' ? me.top : me.bottom - tl;
var yTickEnd = options.position === 'bottom' ? me.top + tl : me.bottom;

helpers.each(me.ticks, function(tick, index) {
var label = (tick && tick.value) || tick;
helpers.each(me._ticks, function(tick, index) {
var label = tick.label;
// If the callback returned a null or undefined value, do not draw this line
if (helpers.isNullOrUndef(label)) {
return;
}

var isLastTick = me.ticks.length === index + 1;
var isLastTick = tickCount === index + 1;

// Since we always show the last tick,we need may need to hide the last shown one before
var shouldSkip = (skipRatio > 1 && index % skipRatio > 0) || (index % skipRatio === 0 && index + skipRatio >= me.ticks.length);
var shouldSkip = (skipRatio > 1 && index % skipRatio > 0) || (index % skipRatio === 0 && index + skipRatio >= tickCount);
if (shouldSkip && !isLastTick || helpers.isNullOrUndef(label)) {
return;
}
Expand Down
55 changes: 34 additions & 21 deletions src/scales/scale.time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,23 @@ function generate(min, max, minor, major, capacity, options) {
return ticks;
}

function ticksFromTimestamps(values, majorUnit) {
var ticks = [];
var i, ilen, value, major;

for (i = 0, ilen = values.length; i < ilen; ++i) {
value = values[i];
major = majorUnit ? value === +moment(value).startOf(majorUnit) : false;

ticks.push({
value: value,
major: major
});
}

return ticks;
}

module.exports = function(Chart) {

var defaultConfig = {
Expand Down Expand Up @@ -515,16 +532,17 @@ module.exports = function(Chart) {
}
}

me.ticks = ticks;
me.min = min;
me.max = max;

// PRIVATE
me._unit = unit;
me._majorUnit = majorUnit;
me._displayFormat = formats[unit];
me._majorDisplayFormat = formats[majorUnit];
me._minorFormat = formats[unit];
me._majorFormat = formats[majorUnit];
me._table = buildLookupTable(ticks, min, max, ticksOpts.mode === 'linear');

return ticksFromTimestamps(ticks, majorUnit);
},

getLabelForIndex: function(index, datasetIndex) {
Expand Down Expand Up @@ -553,31 +571,25 @@ module.exports = function(Chart) {
var options = me.options;
var time = tick.valueOf();
var majorUnit = me._majorUnit;
var majorFormat = me._majorDisplayFormat;
var majorFormat = me._majorFormat;
var majorTime = tick.clone().startOf(me._majorUnit).valueOf();
var major = majorUnit && majorFormat && time === majorTime;
var formattedTick = tick.format(major ? majorFormat : me._displayFormat);
var label = tick.format(major ? majorFormat : me._minorFormat);
var tickOpts = major ? options.ticks.major : options.ticks.minor;
var formatter = helpers.valueOrDefault(tickOpts.callback, tickOpts.userCallback);

if (formatter) {
formattedTick = formatter(formattedTick, index, ticks);
}

return {
value: formattedTick,
major: major,
time: time,
};
return formatter ? formatter(label, index, ticks) : label;
},

convertTicksToLabels: function() {
var ticks = this.ticks;
convertTicksToLabels: function(ticks) {
var labels = [];
var i, ilen;

for (i = 0, ilen = ticks.length; i < ilen; ++i) {
ticks[i] = this.tickFormatFunction(moment(ticks[i]));
labels.push(this.tickFormatFunction(moment(ticks[i].value), i, ticks));
}

return labels;
},

/**
Expand Down Expand Up @@ -610,8 +622,9 @@ module.exports = function(Chart) {
},

getPixelForTick: function(index) {
return index >= 0 && index < this.ticks.length ?
this.getPixelForOffset(this.ticks[index].time) :
var ticks = this.getTicks();
return index >= 0 && index < ticks.length ?
this.getPixelForOffset(ticks[index].value) :
null;
},

Expand Down Expand Up @@ -647,9 +660,9 @@ module.exports = function(Chart) {
getLabelCapacity: function(exampleTime) {
var me = this;

me._displayFormat = me.options.time.displayFormats.millisecond; // Pick the longest format for guestimation
me._minorFormat = me.options.time.displayFormats.millisecond; // Pick the longest format for guestimation

var exampleLabel = me.tickFormatFunction(moment(exampleTime), 0, []).value;
var exampleLabel = me.tickFormatFunction(moment(exampleTime), 0, []);
var tickLabelWidth = me.getLabelWidth(exampleLabel);
var innerWidth = me.isHorizontal() ? me.width : me.height;

Expand Down
4 changes: 2 additions & 2 deletions test/specs/global.deprecations.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,8 @@ describe('Deprecations', function() {
}
});

var ticks = chart.scales.time.ticks.map(function(tick) {
return tick.value;
var ticks = chart.scales.time.getTicks().map(function(tick) {
return tick.label;
});

expect(ticks).toEqual(['8PM', '10PM']);
Expand Down
Loading