Skip to content

Improve offset calculation for scale.offset option #4658

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

Closed
wants to merge 3 commits into from
Closed

Improve offset calculation for scale.offset option #4658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adaeffa
Improve offset calculation for scale.offset option
nagix Aug 15, 2017
839a1d8
Calculate minInterval only when there is one or more data points in t…
nagix Aug 9, 2018
dc9edba
Improve readability and add more comments
nagix Aug 12, 2018
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 @@ -164,7 +164,7 @@ function computeFlexCategoryTraits(index, ruler, options) {
if (prev === null) {
// first data: its size is double based on the next point or,
// if it's also the last data, we use the scale end extremity.
prev = curr - (next === null ? ruler.end - curr : next - curr);
prev = curr - (next === null ? Math.max(curr - ruler.start, ruler.end - curr) * 2 : next - curr);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is getting a bit tricky to follow. I feel like we're trying to hack prev and next to get the desired results for start and size. I wonder if it would be easier if we just computed those directly like:

if (prev == null && next == null) {
  start = ruler.start * (1 - percent / 2);
  size = ruler.end - ruler.start;
} else if (prev == null) {
  start = curr - ((next - curr) / 2) * percent;
  size = ((next - curr) / 2) * percent;
} else if (next == null) {
  start = curr - ((curr - prev) / 2) * percent;
  size =  (curr - prev) * percent;
} else {
  start = curr - ((curr - prev) / 2) * percent;
  size = ((next - prev) / 2) * percent;
}

Note that these formulas probably aren't at all correct. This is just to demonstrate an alternate way of laying out the code

Copy link
Member

Choose a reason for hiding this comment

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

@benmccann your proposal introduces lot of code duplication and actually doesn't look clearer to me since it's hard to follow all the if / else. The current implementation is not a "hack" of prev / next but an adjustment when they are not defined. Comments explain pretty well what's going on so please don't reformat this part of the code.

}

if (next === null) {
Expand Down
76 changes: 56 additions & 20 deletions src/scales/scale.time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,31 +358,67 @@ function generate(min, max, capacity, options) {
}

/**
* Returns the right and left offsets from edges in the form of {left, right}.
* Returns the right and left offsets from the edges of the chart in the form of {left, right}
* where each values is a relative width to the scale and ranges between 0 and 1.
* They add extra margins on the both sides by scaling down the original scale.
* Offsets are typically used for bar charts. They are calculated based on intervals
* between the data points to keep the leftmost and rightmost bars from being cut.
* Offsets are added when the `offset` option is true.
*/
function computeOffsets(table, ticks, min, max, options) {
function computeOffsets(table, ticks, data, min, max, options) {
var pos = [];
var minInterval = 1;
var timeOpts = options.time;
var barThickness = options.barThickness;
Copy link
Member

Choose a reason for hiding this comment

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

Accessing barThickness from the time scale doesn't seem ideal because it correlates the bar controller and this scale, which is something we are trying to remove / avoid. Actually, the barThickness should not be part of the scale options.

var left = 0;
var right = 0;
var upper, lower;

if (options.offset && ticks.length) {
if (!options.time.min) {
upper = ticks.length > 1 ? ticks[1] : max;
lower = ticks[0];
left = (
interpolate(table, 'time', upper, 'pos') -
interpolate(table, 'time', lower, 'pos')
) / 2;
var i, ilen, curr, prev, length, width;

if (!options.offset) {
return {left: 0, right: 0};
}

data.forEach(function(timestamp) {
if (timestamp >= min && timestamp <= max) {
pos.push(interpolate(table, 'time', timestamp, 'pos'));
}
if (!options.time.max) {
upper = ticks[ticks.length - 1];
lower = ticks.length > 1 ? ticks[ticks.length - 2] : min;
right = (
interpolate(table, 'time', upper, 'pos') -
interpolate(table, 'time', lower, 'pos')
) / 2;
});

length = pos.length;
if (!length) {
return {left: 0, right: 0};
}

if (!barThickness) {
// Calculate minInterval
[data, ticks].forEach(function(timestamps) {
for (i = 0, ilen = timestamps.length; i < ilen; ++i) {
curr = interpolate(table, 'time', timestamps[i], 'pos');
minInterval = i > 0 ? Math.min(minInterval, curr - prev) : minInterval;
prev = curr;
}
});
}

if (!timeOpts.min) {
if (length === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

actually I just thought of one last thing. can we move setting width outside to occur just before the if statement? I think that would allow removing the duplicate code here and in the other if statement just below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. width for the left and right offset needs to be calculated separately, and the calculation depends on the length of pos and barThickness.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. Nevermind

width = (1 - pos[0]) * 2;
} else if (barThickness) {
width = pos[1] - pos[0];
} else {
width = minInterval;
}
left = Math.max(width / 2 - pos[0], 0);
}
if (!timeOpts.max) {
if (length === 1) {
width = pos[0] * 2;
} else if (barThickness) {
width = pos[length - 1] - pos[length - 2];
} else {
width = minInterval;
}
right = Math.max(width / 2 - (1 - pos[length - 1]), 0);
}

return {left: left, right: right};
Expand Down Expand Up @@ -643,7 +679,7 @@ module.exports = function() {
me._unit = timeOpts.unit || determineUnitForFormatting(ticks, timeOpts.minUnit, me.min, me.max);
me._majorUnit = determineMajorUnit(me._unit);
me._table = buildLookupTable(me._timestamps.data, min, max, options.distribution);
me._offsets = computeOffsets(me._table, ticks, min, max, options);
me._offsets = computeOffsets(me._table, ticks, me._timestamps.data, min, max, options);
me._labelFormat = determineLabelFormat(me._timestamps.data, timeOpts);

return ticksFromTimestamps(ticks, me._majorUnit);
Expand Down
Binary file modified test/fixtures/controller.bar/bar-thickness-offset.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
29 changes: 24 additions & 5 deletions test/specs/scale.time.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1276,16 +1276,35 @@ describe('Time scale tests', function() {
var chart = this.chart;
var scale = chart.scales.x;
var options = chart.options.scales.xAxes[0];
var minInterval;

options.offset = true;
chart.update();

var numTicks = scale.ticks.length;
var firstTickInterval = scale.getPixelForTick(1) - scale.getPixelForTick(0);
var lastTickInterval = scale.getPixelForTick(numTicks - 1) - scale.getPixelForTick(numTicks - 2);
if (source === 'auto' && distribution === 'series') {
minInterval = scale.getPixelForTick(5) - scale.getPixelForTick(4);
} else {
minInterval = scale.getPixelForValue('2020') - scale.getPixelForValue('2019');
}

expect(scale.getPixelForValue('2017')).toBeCloseToPixel(scale.left + minInterval / 2);
expect(scale.getPixelForValue('2042')).toBeCloseToPixel(scale.left + scale.width - minInterval / 2);
});

it ('should add offset from the edges if offset is true and barThickness is "flex"', function() {
var chart = this.chart;
var scale = chart.scales.x;
var options = chart.options.scales.xAxes[0];

options.offset = true;
options.barThickness = 'flex';
chart.update();

var firstInterval = scale.getPixelForValue('2019') - scale.getPixelForValue('2017');
var lastInterval = scale.getPixelForValue('2042') - scale.getPixelForValue('2025');

expect(scale.getPixelForValue('2017')).toBeCloseToPixel(scale.left + firstTickInterval / 2);
expect(scale.getPixelForValue('2042')).toBeCloseToPixel(scale.left + scale.width - lastTickInterval / 2);
expect(scale.getPixelForValue('2017')).toBeCloseToPixel(scale.left + firstInterval / 2);
expect(scale.getPixelForValue('2042')).toBeCloseToPixel(scale.left + scale.width - lastInterval / 2);
});

it ('should not add offset if min and max extend the labels range', function() {
Expand Down