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

Added 'angle' option to Polar Charts #5279

Merged
merged 9 commits into from Jun 17, 2018
Merged

Added 'angle' option to Polar Charts #5279

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
94bf815
added 'angle' option to polar charts. image comparison test is work i…
Feb 18, 2018
8c8b864
removed unnecessary variable assignment
Feb 18, 2018
ff432f6
code cleanup based on PR; for 'angle' option on polarCharts
Feb 20, 2018
debbf75
Made polar chart image comparison test pass by removing debug flag. A…
Feb 22, 2018
3f1eb49
Removed visibleCount computation in polar chart
Feb 22, 2018
86419e2
split out code related to updating the radius in polar chart's update…
Mar 1, 2018
66e8e53
made updateRadius method private
Mar 3, 2018
45d3bc0
fix linting error
Mar 3, 2018
4a42e41
updated polar charts to read custom angles from "chart.options.elemen…
slinhart Jun 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 39 additions & 7 deletions src/controllers/controller.polarArea.js
View file
Open in desktop
Expand Up @@ -123,7 +123,12 @@ module.exports = function(Chart) {
var meta = me.getMeta();
var opts = chart.options;
var arcOpts = opts.elements.arc;
var dataset = me.getDataset();
var minSize = Math.min(chartArea.right - chartArea.left, chartArea.bottom - chartArea.top);
var start = opts.startAngle || 0;
var starts = [];
var angles = [];
Copy link
Member

Choose a reason for hiding this comment

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

I changed my previous comment a bit too late:

var starts = me._starts = [];
var angles = me._angles = [];

And lines 148-149 can be removed :)

var i, ilen, angle;
chart.outerRadius = Math.max((minSize - arcOpts.borderWidth / 2) / 2, 0);
chart.innerRadius = Math.max(opts.cutoutPercentage ? (chart.outerRadius / 100) * (opts.cutoutPercentage) : 1, 0);
chart.radiusLength = (chart.outerRadius - chart.innerRadius) / chart.getVisibleDatasetCount();
Expand All @@ -133,6 +138,16 @@ module.exports = function(Chart) {

meta.count = me.countVisibleElements();

for (i = 0, ilen = dataset.data.length; i < ilen; i++) {
starts[i] = start;
angle = me._computeAngle(i);
angles[i] = angle;
start += angle;
}

me._starts = starts;
me._angles = angles;

helpers.each(meta.data, function(arc, index) {
Copy link
Member

Choose a reason for hiding this comment

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

To avoid accessing object properties at every iteration, I would use local variables:

var minSize = ...
var start = opts.startAngle || 0;
var starts = me._starts = [];
var angles = me._angles = [];
var i, ilen;

//...

meta.count = me.countVisibleElements();

for (i = 0, ilen = dataset.data.length; i < ilen; i++) {
  angle = me._computeAngle(i);
  angles[i] = angle;
  starts[i] = start;
  start += angle
}

Edit: var starts = me._starts = [] instead of var starts = []; .... me._starts = starts (same for angles)

me.updateElement(arc, index, reset);
});
Expand All @@ -147,7 +162,6 @@ module.exports = function(Chart) {
var scale = chart.scale;
var labels = chart.data.labels;

var circumference = me.calculateCircumference(dataset.data[index]);
var centerX = scale.xCenter;
var centerY = scale.yCenter;

Expand All @@ -164,8 +178,8 @@ module.exports = function(Chart) {
// var negHalfPI = -0.5 * Math.PI;
var datasetStartAngle = opts.startAngle;
var distance = arc.hidden ? 0 : scale.getDistanceFromCenterForValue(dataset.data[index]);
var startAngle = datasetStartAngle + (circumference * visibleCount);
Copy link
Member

Choose a reason for hiding this comment

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

Travis complains that visibleCount is not anymore used and it seems that the whole block from line 167 to 173 can be removed. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, thanks "Travis" 😅

var endAngle = startAngle + (arc.hidden ? 0 : circumference);
var startAngle = me._starts[index];
var endAngle = startAngle + (arc.hidden ? 0 : me._angles[index]);

var resetRadius = animationOpts.animateScale ? 0 : scale.getDistanceFromCenterForValue(dataset.data[index]);

Expand Down Expand Up @@ -211,12 +225,30 @@ module.exports = function(Chart) {
return count;
},

calculateCircumference: function(value) {
_computeAngle: function(index) {
Copy link
Member

Choose a reason for hiding this comment

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

We started to be even more explicit about private stuff by adding the following comment above:

/**
 * @private
 */
_computeAngle: function(index) {

var me = this;
var count = this.getMeta().count;
if (count > 0 && !isNaN(value)) {
return (2 * Math.PI) / count;
var dataset = me.getDataset();
var meta = me.getMeta();
var value = dataset.data[index];
var isHidden = meta.data[index].hidden;

if (count <= 0 || isNaN(value) || isHidden) {
Copy link
Member

Choose a reason for hiding this comment

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

When referenced only one time, I think it's preferable to avoid local variable for short assignment. Also, I don't think we need to check count <= 0 anymore because (if I'm not wrong) if count <= 0 then the value is either NaN or hidden.

if (isNaN(dataset.data[index]) || meta.data[index].hidden) {
  return 0;
}

return 0;
}
return 0;

// Scriptable options
var context = {
chart: me.chart,
dataIndex: index,
dataset: dataset,
datasetIndex: me.index
};

return helpers.options.resolve([
me.chart.options.angle,
(2 * Math.PI) / count
], context, index);
}
});
};
32 changes: 32 additions & 0 deletions test/fixtures/controller.polarArea/angle-array.json
View file
Open in desktop
@@ -0,0 +1,32 @@
{
"debug": true,
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed, it's only used for debugging and is likely the reason why unit tests are failing on Travis.

"config": {
"debug": true,
Copy link
Member

Choose a reason for hiding this comment

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

This one can be removed because it doesn't do anything

"type": "polarArea",
"data": {
"labels": ["A", "B", "C", "D", "E"],
"datasets": [{
"data": [11, 16, 21, 7, 10],
"backgroundColor": [
"rgba(255, 99, 132, 0.8)",
"rgba(54, 162, 235, 0.8)",
"rgba(255, 206, 86, 0.8)",
"rgba(75, 192, 192, 0.8)",
"rgba(153, 102, 255, 0.8)",
"rgba(255, 159, 64, 0.8)"
]
}]
},
"options": {
"angle": [
1.0566, 1.7566, 1.0566, 2.1566, 0.2566
],
"responsive": false,
"legend": false,
"title": false,
"scale": {
"display": false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We should remove all UI that are not part of the tests, which include the labels and the "grid lines". Maybe adding scale: { display: false } is enough.

Copy link
Member

Choose a reason for hiding this comment

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

I would also pick a more accentuated and different background color for each slices to make sure it fails if there is overlapping elements.

}
}
Binary file added test/fixtures/controller.polarArea/angle-array.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
29 changes: 29 additions & 0 deletions test/fixtures/controller.polarArea/angle-undefined.json
View file
Open in desktop
@@ -0,0 +1,29 @@
{
"debug": true,
"config": {
"debug": true,
"type": "polarArea",
"data": {
"labels": ["A", "B", "C", "D", "E"],
"datasets": [{
"data": [11, 16, 21, 7, 10],
"backgroundColor": [
"rgba(255, 99, 132, 0.8)",
"rgba(54, 162, 235, 0.8)",
"rgba(255, 206, 86, 0.8)",
"rgba(75, 192, 192, 0.8)",
"rgba(153, 102, 255, 0.8)",
"rgba(255, 159, 64, 0.8)"
]
}]
},
"options": {
"responsive": false,
"legend": false,
"title": false,
"scale": {
"display": false
}
}
}
}
Binary file added test/fixtures/controller.polarArea/angle-undefined.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 2 additions & 0 deletions test/specs/controller.polarArea.tests.js
View file
Open in desktop
@@ -1,3 +1,5 @@
describe('auto', jasmine.specsFromFixtures('controller.polarArea'));

describe('Chart.controllers.polarArea', function() {
it('should be constructed', function() {
var chart = window.acquireChart({
Expand Down