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.

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

Added 'angle' option to Polar Charts #5279

Merged
merged 9 commits into from
Jun 17, 2018
Merged

Conversation

slinhart
Copy link
Contributor

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.

@etimberg
Copy link
Member

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.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

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:

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

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

@@ -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 = ...

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

@simonbrunel simonbrunel Feb 18, 2018

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)

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

"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.

"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.

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

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

  //...
}

@@ -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++) {
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?

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

@slinhart
Copy link
Contributor Author

@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.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

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.

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

@@ -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) {

@@ -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.

{
"debug": true,
"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

…lso explicitly marked _computeAngle as private.
@@ -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);
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" 😅

});
},

updateRadius: function() {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be private:

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

@etimberg
Copy link
Member

@simonbrunel to answer your questions:

  1. 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.
  2. I don't think that's necessary for the same reasons as above.
  3. Yes
  4. I would say radians to match other APIs in the polar area chart. We can change to degrees in v3

etimberg
etimberg previously approved these changes Apr 22, 2018
@simonbrunel
Copy link
Member

@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
Copy link
Member

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

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

@simonbrunel Sure, I'll update that

…ts.arc.angle" instead of "chart.options.angle"
@etimberg etimberg merged commit a0a195f into chartjs:master Jun 17, 2018
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
* 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"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants