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 a bar overlap issue with unordered labels #4674

Closed
wants to merge 2 commits into from
Closed

Fix a bar overlap issue with unordered labels #4674

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
281681e
Fix a bar overlap issue with unordered labels
nagix Aug 16, 2017
b4f7d48
Add a comment on the ordered pixels and the index translation table
nagix Aug 20, 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
29 changes: 24 additions & 5 deletions src/controllers/controller.bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,17 +243,33 @@ module.exports = function(Chart) {
var stackCount = me.getStackCount();
var datasetIndex = me.index;
var pixels = [];
var orderedPixels = [];
var order = [];
var isHorizontal = scale.isHorizontal();
var start = isHorizontal ? scale.left : scale.top;
var end = start + (isHorizontal ? scale.width : scale.height);
var i, ilen;
var i, ilen, pixel;

for (i = 0, ilen = me.getMeta().data.length; i < ilen; ++i) {
pixels.push(scale.getPixelForValue(null, i, datasetIndex));
pixel = scale.getPixelForValue(null, i, datasetIndex);
if (pixel !== undefined) {
pixels.push({index: i, value: pixel});
}
}

// The ordered pixels and the index translation table are stored in the ruler.
// They are used for bar width calculation.
pixels.sort(function(a, b) {
return a.value - b.value;
});
for (i = 0, ilen = pixels.length; i < ilen; ++i) {
orderedPixels.push(pixels[i].value);
order[pixels[i].index] = i;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this index translation. I feel like we should sort the data at the beginning but that has a number of downsides. It changes the data the user specified and also requires adjusting the data arrays in each dataset accordingly.

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 don't think we need to adjust the data arrays in datasets because the order array translates an original index into an ordered index, and the ordered indexes and pixels are only used for bar width calculation. Do you have any specific problematic cases in mind?

Copy link
Member

Choose a reason for hiding this comment

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

I think the problematic cases are mostly for debugging purposes. It's mostly code style / personal preference. But i will agree that this is the simplest way to achieve this

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 added comments to avoid confusions in the future.

}

return {
pixels: pixels,
pixels: orderedPixels,
order: order,
start: start,
end: end,
stackCount: stackCount,
Expand Down Expand Up @@ -314,11 +330,14 @@ module.exports = function(Chart) {
var options = ruler.scale.options;
var stackIndex = me.getStackIndex(datasetIndex);
var pixels = ruler.pixels;
var base = pixels[index];
var length = pixels.length;
var start = ruler.start;
var end = ruler.end;
var leftSampleSize, rightSampleSize, leftCategorySize, rightCategorySize, fullBarSize, size;
var base, leftSampleSize, rightSampleSize, leftCategorySize, rightCategorySize, fullBarSize, size;

// The index is translated so that it points to the ordered pixel
index = ruler.order[index];
base = pixels[index];

if (length === 1) {
leftSampleSize = base > start ? base - start : end - base;
Expand Down