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 2 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
43 changes: 36 additions & 7 deletions src/controllers/controller.polarArea.js
View file
Open in desktop
Expand Up @@ -116,13 +116,18 @@ module.exports = function(Chart) {

linkScales: helpers.noop,

_starts: {},

_angles: {},
Copy link
Member

Choose a reason for hiding this comment

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

extend() (alias of inherit()) augments the prototype, _starts and _angles shouldn't be declared that way else they will be shared between all polar controller instances. Actually, I don't think they need to be pre-declared at all since you need to reset both in update() (me._starts = [] and me._angles = []).


update: function(reset) {
var me = this;
var chart = me.chart;
var chartArea = chart.chartArea;
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);
chart.outerRadius = Math.max((minSize - arcOpts.borderWidth / 2) / 2, 0);
chart.innerRadius = Math.max(opts.cutoutPercentage ? (chart.outerRadius / 100) * (opts.cutoutPercentage) : 1, 0);
Expand All @@ -133,6 +138,14 @@ module.exports = function(Chart) {

meta.count = me.countVisibleElements();

var start = opts.startAngle || 0;
Copy link
Member

Choose a reason for hiding this comment

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

[minifier] I would move this declaration right after var minSize = ...

for (var i = 0; i < meta.count; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we only want to iterate until meta.count representing the number of visible elements, which are not necessary the first elements?

me._starts[i] = start;
var angle = me.computeAngle(dataset.data[i], i);
Copy link
Member

Choose a reason for hiding this comment

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

[minifier/optim] I would move var i, angle; uninitialized declarations right after var start ....

me._angles[i] = angle;
start += angle;
}

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 +160,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 +176,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 +223,29 @@ module.exports = function(Chart) {
return count;
},

calculateCircumference: function(value) {
computeAngle: function(value, index) {
Copy link
Member

Choose a reason for hiding this comment

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

calculateCircumference was previously implicitly part of the public API, @etimberg do you think it's fine to rename it?

If we agree to rename it, then the new one should be explicitly private:

/**
 * @private
 */
_computeAngle: function(value, index) {
  // ...
}

var me = this;
var count = this.getMeta().count;
if (count > 0 && !isNaN(value)) {
return (2 * Math.PI) / count;
if (count <= 0 || isNaN(value)) {
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 we also need to return 0 is the element is hidden

return 0;
}
return 0;

var defaultAngle = (2 * Math.PI) / count;
var angleOption = me.chart.options.angle || defaultAngle;
var resolve = helpers.options.resolve;
var dataset = me.getDataset();
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 change the method signature to only pass index as argument and read the value here:

_computeAngle: function(index) {
   //...

  var dataset = me.getDataset();
  var value = dataset.data[index];

  //...
}


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

return resolve([
Copy link
Member

Choose a reason for hiding this comment

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

resolve is used only one time here so no need for the extra local variable. Also, I think we should make the defaultAngle part of the resolve process:

return helpers.options.resolve([
   // potentially: dataset.angle
   // potentially: me.chart.options.elements.arc.angle
   me.chart.options.angle,
   (2 * Math.PI) / count
], context, index);

This is because if me.chart.options.angle is undefined, or if an array and contains an undefined value or if a function and return undefined, then it will fallback to (2 * Math.PI) / count.

angleOption
], context, index);
}
});
};
21 changes: 21 additions & 0 deletions test/fixtures/controller.polarArea/angle-array.json
View file
Open in desktop
@@ -0,0 +1,21 @@
{
"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]
}]
},
"options": {
"angle": [
1.0566, 1.7566, 1.0566, 2.1566, 0.2566
],
"responsive": false,
"legend": false,
"title": 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.
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