Added 'angle' option to Polar Charts #5279

slinhart · 2018-02-18T00:25:30Z

I came across this inactive Issue and picked it up.

The image comparison test I added is not passing even though the diff is 0% 🤷‍♂️
Also, I imagine this will need a documentation update. Should the "Scriptable" column be added to the options table for Polar Charts? The first sentence for Polar Charts might also need updated since it reads

Polar area charts are similar to pie charts, but each segment has the same angle - the radius of the segment differs depending on the value.

…n progress; not currently passing

etimberg · 2018-02-18T02:13:17Z

I looked at the test failure quickly. Looks like 1px differs against the original image. Not sure what the different pixel is since it didn't really stand out on the diff image. I noticed that labels on the index were still present. Perhaps removing them as well will remove the single pixel diff.

simonbrunel

Thanks @slinhart for reviving #4753! It looks really good. Lot of comments, but most are pretty minor and easy to fix. For the unit tests, it's certainly due to the labels because font rendering is not stable enough between different browsers, so the best is to remove them since we are not testing that feature anyway. (1px difference is not enough to reject the test, if I'm not wrong, there is 0.1% tolerance of the total number of pixels)

I would add 2 more unit tests: angle-undefined.json and angle-scriptable.json.

For the scriptable, we could do something like:

data: [1, 2, 4, 3, 5],
//...
angle: function(context) {
    var value = context.dataset.data[context.dataIndex];
    return 2 * Math.PI / (value / 15);
}

@etimberg About the API:

do we want to support that option at the element level (options.elements.arc.angle)?
is it fine to add angle at the root (options.angle) or element level is enough?
should we support angle at the dataset level (dataset.angle)?
should we handle degrees or radians?

simonbrunel · 2018-02-18T10:10:57Z

src/controllers/controller.polarArea.js

@@ -116,13 +116,18 @@ module.exports = function(Chart) {

 		linkScales: helpers.noop,

+		_starts: {},
+
+		_angles: {},


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 = []).

simonbrunel · 2018-02-18T10:12:03Z

src/controllers/controller.polarArea.js

@@ -133,6 +138,14 @@ module.exports = function(Chart) {

 			meta.count = me.countVisibleElements();

+			var start = opts.startAngle || 0;


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

simonbrunel · 2018-02-18T10:14:25Z

src/controllers/controller.polarArea.js

+			var start = opts.startAngle || 0;
+			for (var i = 0; i < meta.count; i++) {
+				me._starts[i] = start;
+				var angle = me.computeAngle(dataset.data[i], i);


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

simonbrunel · 2018-02-18T10:23:31Z

src/controllers/controller.polarArea.js

+				me._angles[i] = angle;
+				start += angle;
+			}
+
 			helpers.each(meta.data, function(arc, index) {


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)

simonbrunel · 2018-02-18T10:25:06Z

src/controllers/controller.polarArea.js

@@ -211,12 +223,29 @@ module.exports = function(Chart) {
 			return count;
 		},

-		calculateCircumference: function(value) {
+		computeAngle: function(value, index) {


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) { // ... }

simonbrunel · 2018-02-18T10:38:02Z

test/fixtures/controller.polarArea/angle-array.json

+            "responsive": false,
+            "legend": false,
+            "title": false
+        }


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.

simonbrunel · 2018-02-18T10:40:39Z

test/fixtures/controller.polarArea/angle-array.json

+            "responsive": false,
+            "legend": false,
+            "title": false
+        }


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

simonbrunel · 2018-02-18T11:11:00Z

src/controllers/controller.polarArea.js

+			var defaultAngle = (2 * Math.PI) / count;
+			var angleOption = me.chart.options.angle || defaultAngle;
+			var resolve = helpers.options.resolve;
+			var dataset = me.getDataset();


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]; //... }

simonbrunel · 2018-02-18T11:25:51Z

src/controllers/controller.polarArea.js

@@ -133,6 +138,14 @@ module.exports = function(Chart) {

 			meta.count = me.countVisibleElements();

+			var start = opts.startAngle || 0;
+			for (var i = 0; i < meta.count; i++) {


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

simonbrunel · 2018-02-18T11:27:16Z

src/controllers/controller.polarArea.js

 			var count = this.getMeta().count;
-			if (count > 0 && !isNaN(value)) {
-				return (2 * Math.PI) / count;
+			if (count <= 0 || isNaN(value)) {


I think we also need to return 0 is the element is hidden

slinhart · 2018-02-20T01:23:24Z

@simonbrunel Thank you very much for the detailed review. I've updated the code based on your feedback. As far as the tests, I'm still having trouble getting those to pass. I've removed all the text from the chart and the image comparison is still failing (displays 0px diff in debug view). I added another test for 'undefined', but for the 'scriptiable' test I was unsure how to define this in the JSON file.

simonbrunel

Thanks @slinhart, just a few more minor changes and I think we are good.

... the image comparison is still failing ...

I missed that in your initial PR but it's due to the debug: true in the JSON files which force the comparison to fail and thus display the output images. Removing these lines should make Travis happy.

... for the 'scriptiable' test I was unsure how to define this in the JSON file ...

You right, I forgot that this repo doesn't support JavaScript fixtures yet (as in the datalabels plugin). Let's skip that test for now.

simonbrunel · 2018-02-20T08:28:19Z

src/controllers/controller.polarArea.js

 			var minSize = Math.min(chartArea.right - chartArea.left, chartArea.bottom - chartArea.top);
+			var start = opts.startAngle || 0;
+			var starts = [];
+			var angles = [];


I changed my previous comment a bit too late:

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

And lines 148-149 can be removed :)

simonbrunel · 2018-02-20T08:34:59Z

src/controllers/controller.polarArea.js

+			var value = dataset.data[index];
+			var isHidden = meta.data[index].hidden;
+
+			if (count <= 0 || isNaN(value) || isHidden) {


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

simonbrunel · 2018-02-20T08:36:49Z

src/controllers/controller.polarArea.js

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

-		calculateCircumference: function(value) {
+		_computeAngle: function(index) {


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

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

simonbrunel · 2018-02-20T08:37:36Z

test/fixtures/controller.polarArea/angle-array.json

@@ -0,0 +1,32 @@
+{
+    "debug": true,


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

simonbrunel · 2018-02-20T08:39:15Z

test/fixtures/controller.polarArea/angle-array.json

+{
+    "debug": true,
+    "config": {
+        "debug": true,


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

…lso explicitly marked _computeAngle as private.

simonbrunel · 2018-02-22T09:56:58Z

src/controllers/controller.polarArea.js

@@ -164,8 +175,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);


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?

Oh right, thanks "Travis" 😅

… function, into it's own 'updateRadius' function

simonbrunel · 2018-03-01T09:12:26Z

src/controllers/controller.polarArea.js

+			});
+		},
+
+		updateRadius: function() {


This needs to be private:

/** * @private */ _updateRadius: function() { // ...

etimberg · 2018-03-10T16:21:05Z

@simonbrunel to answer your questions:

What happens when multiple datasets exist in the polar area chart? I think we only support 1 making this unnecessary. If multiple datasets work, then we should support this.
I don't think that's necessary for the same reasons as above.
Yes
I would say radians to match other APIs in the polar area chart. We can change to degrees in v3

simonbrunel · 2018-05-22T20:30:34Z

@etimberg I think this PR is almost ready to be merged, the only reason it's not is the choice of the option, chart.options.angle at the root, which could be confusing with other concepts. Should we prefer chart.options.element.arc.angle since this feature impact the arc angle?

etimberg · 2018-05-23T01:09:07Z

@simonbrunel I think chart.options.element.arc.angle is fine since it keeps it away from other things.

simonbrunel

@slinhart can we change the code to use chart.options.elements.arc.angle instead of chart.options.angle?

We should also add the associated docs.

slinhart · 2018-05-29T14:42:03Z

@simonbrunel Sure, I'll update that

…ts.arc.angle" instead of "chart.options.angle"

* added 'angle' option to polar charts. image comparison test is work in progress; not currently passing * removed unnecessary variable assignment * code cleanup based on PR; for 'angle' option on polarCharts * Made polar chart image comparison test pass by removing debug flag. Also explicitly marked _computeAngle as private. * Removed visibleCount computation in polar chart * split out code related to updating the radius in polar chart's update function, into it's own 'updateRadius' function * made updateRadius method private * fix linting error * updated polar charts to read custom angles from "chart.options.elements.arc.angle" instead of "chart.options.angle"

Shayne Linhart added 2 commits February 17, 2018 17:06

added 'angle' option to polar charts. image comparison test is work i…

94bf815

…n progress; not currently passing

removed unnecessary variable assignment

8c8b864

etimberg requested a review from simonbrunel February 18, 2018 01:47

simonbrunel requested changes Feb 18, 2018

View reviewed changes

simonbrunel added the type: enhancement label Feb 18, 2018

simonbrunel added this to the Version 2.8 milestone Feb 18, 2018

code cleanup based on PR; for 'angle' option on polarCharts

ff432f6

simonbrunel requested changes Feb 20, 2018

View reviewed changes

Made polar chart image comparison test pass by removing debug flag. A…

debbf75

…lso explicitly marked _computeAngle as private.

simonbrunel reviewed Feb 22, 2018

View reviewed changes

Shayne Linhart added 2 commits February 22, 2018 07:57

Removed visibleCount computation in polar chart

3f1eb49

split out code related to updating the radius in polar chart's update…

86419e2

… function, into it's own 'updateRadius' function

simonbrunel requested changes Mar 2, 2018

View reviewed changes

Shayne Linhart added 2 commits March 3, 2018 10:22

made updateRadius method private

66e8e53

fix linting error

45d3bc0

simonbrunel approved these changes Mar 4, 2018

View reviewed changes

simonbrunel requested a review from etimberg March 4, 2018 09:18

benmccann mentioned this pull request Apr 22, 2018

Consider whether we want degrees and/or radians #5438

Closed

etimberg previously approved these changes Apr 22, 2018

View reviewed changes

simonbrunel requested changes May 26, 2018

View reviewed changes

updated polar charts to read custom angles from "chart.options.elemen…

4a42e41

…ts.arc.angle" instead of "chart.options.angle"

slinhart dismissed etimberg’s stale review via 4a42e41 June 5, 2018 00:32

simonbrunel approved these changes Jun 16, 2018

View reviewed changes

etimberg merged commit a0a195f into chartjs:master Jun 17, 2018

nagix mentioned this pull request Jan 2, 2019

Polar area: align start angle with angleLines #5919

Closed

vpetersson mentioned this pull request Mar 7, 2020

[Snyk] Upgrade chart.js from 2.7.3 to 2.9.3 WoTTsecurity/api#764

Merged

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added 'angle' option to Polar Charts #5279

Added 'angle' option to Polar Charts #5279

slinhart commented Feb 18, 2018

etimberg commented Feb 18, 2018

simonbrunel left a comment •

edited

simonbrunel Feb 18, 2018

simonbrunel Feb 18, 2018

simonbrunel Feb 18, 2018

simonbrunel Feb 18, 2018 •

edited

simonbrunel Feb 18, 2018

simonbrunel Feb 18, 2018

simonbrunel Feb 18, 2018

simonbrunel Feb 18, 2018

simonbrunel Feb 18, 2018

simonbrunel Feb 18, 2018

slinhart commented Feb 20, 2018

simonbrunel left a comment

simonbrunel Feb 20, 2018

simonbrunel Feb 20, 2018

simonbrunel Feb 20, 2018

simonbrunel Feb 20, 2018

simonbrunel Feb 20, 2018

simonbrunel Feb 22, 2018

slinhart Feb 22, 2018

simonbrunel Mar 1, 2018

etimberg commented Mar 10, 2018

simonbrunel commented May 22, 2018

etimberg commented May 23, 2018

simonbrunel left a comment

slinhart commented May 29, 2018

		@@ -133,6 +138,14 @@ module.exports = function(Chart) {

		meta.count = me.countVisibleElements();

		var start = opts.startAngle \|\| 0;

Added 'angle' option to Polar Charts #5279

Added 'angle' option to Polar Charts #5279

Conversation

slinhart commented Feb 18, 2018

etimberg commented Feb 18, 2018

simonbrunel left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel Feb 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinhart commented Feb 20, 2018

simonbrunel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg commented Mar 10, 2018

simonbrunel commented May 22, 2018

etimberg commented May 23, 2018

simonbrunel left a comment

Choose a reason for hiding this comment

slinhart commented May 29, 2018

simonbrunel left a comment •

edited

simonbrunel Feb 18, 2018 •

edited