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

Improve offset calculation for scale.offset option #4658

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 2 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
68 changes: 48 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 @@ -361,27 +361,55 @@ function generate(min, max, capacity, options) {
* Returns the right and left offsets from edges in the form of {left, right}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify more what this offset it. What is it that gets offset and why does it need to be offset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this make sense?

/**
 * Returns the right and left offsets from edges 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.
 */

Copy link
Contributor

Choose a reason for hiding this comment

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

Super helpful!

To clarify:
Are these offsets for each bar or only the leftmost and rightmost bar? I.e. can you update "edges" to specify the edge of the chart or the edge of the category

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it's "the edges of the chart".

* 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;
}
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;
var i, ilen, curr, prev, length, width;

if (options.offset) {
data.forEach(function(timestamp) {
if (timestamp >= min && timestamp <= max) {
pos.push(interpolate(table, 'time', timestamp, 'pos'));
}
});

length = pos.length;
if (length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be combined with the enclosing if statement? I.e. if (options.offset && data.length)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if data is not empty, we need to use the subset of data, which only contains timestamps between min and max, so checking pos.length is needed. But, we can move this if (length) statement before the calculation of minInterval.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you're right. Maybe we could also invert the check. E.g.:

		data.forEach(function(timestamp) {
			if (timestamp >= min && timestamp <= max) {
				pos.push(interpolate(table, 'time', timestamp, 'pos'));
			}
		});

		length = pos.length;
		if (!length) {
			return;
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this function needs to return an object, I will just move the check.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could make the code slightly easier to read and remove a lot of indentation if we reversed this if statement:

if (!length) {
  return {left: left, right: right};
}

You could do the same just above with the options.offset check as well which would remove the need for two levels of indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Will update it.

// Calculate minInterval
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 a bit nit picky, but I would actually move this comment down a line (i.e. right above [data, ticks].forEach(function(timestamps) {) because right now it's not clear if it's referring to the entire block of code inside the if (length) statement of just to the if (!barThickness) section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, agreed.

if (!barThickness) {
[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.

What if length === 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case that there is only one data point within the range. length === 0 is already filtered out here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I missed that it was filtered out up above

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);
}
}
}

Expand Down Expand Up @@ -643,7 +671,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