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

Add custom angles to polar area graph #4753

Closed
wants to merge 10 commits into from
Closed

Add custom angles to polar area graph #4753

wants to merge 10 commits into from

Conversation

MarioIannotta
Copy link

Old Behavior

Every angle is 360/#values °

New Behavior

Provide custom value for each angle of the polar graph.

Solution

  	var options = {
 		type: 'polarArea',
 		data: {
 		labels: ["A", "B", "C", "D", "E"],
 		datasets: [{}]
 		},
 		options: {
 			customAngles: [
 				1.0566, 1.7566, 1.0566, 2.1566, 0.2566
 			]
 		}
 	}

Live demo:
https://codepen.io/anon/pen/ZXEmQm

@simonbrunel
Copy link
Member

@MarioIannotta please don't create a new PR for every update, simply push to your branch new changes.

@simonbrunel
Copy link
Member

simonbrunel commented Sep 13, 2017

Referring to #4752, but maybe you closed this one for another reason :)

@MarioIannotta
Copy link
Author

Yeah, the reason is that I was sleepy :)
Let's use this one :)

@MarioIannotta MarioIannotta reopened this Sep 13, 2017
++visibleCount;
}
}
var startAngle = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

this project has a convention of declaring all the variables at the beginning of a function as it apparently improves code minimization. can you move the declaration of startAngle and also i to occur with previousAngle and visibleCount?

Move "i" and "startAngle" at the beginning of the function.
var previousAngle = 0;
var visibleCount = 0;
var startAngle = 0;
var i = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be just var i since it's initialized to 0 below

@benmccann
Copy link
Contributor

Your codepen example doesn't seem to be working

@MarioIannotta
Copy link
Author

Sorry, there was an issue with github hosting and codepen, it's fixed now.

@benmccann
Copy link
Contributor

Thanks for fixing the codepen. It helps demonstrate your PR.

I'm not sure about the name customAngles because really all options are custom, so we could put custom in many of the options. Perhaps calling it just angles would be sufficient?

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.

I agree with @benmccann, I would not use custom* naming but also I would implement this new option as scriptable, and so call it angle (singular):

  • if undefined (default), compute the angle based on the current formula (360/value?)
  • if number, same angle for all values (might not be useful, but consistent with other scriptable options)
  • if array, each item represent the angle for the value at the same index (your implementation)
  • if function, use the returned value as angle for the current value

You will need to use the resolve helper to compute the actual value for each element (see the bubble chart for example).

If you don't feel to implement the scriptable behavior, then let's implement the undefined, number and array versions with angle as option name (@etimberg thoughts?)

var startAngle = 0;
var i = 0;

for (i = 0; i < index; ++i) {
Copy link
Member

@simonbrunel simonbrunel Oct 1, 2017

Choose a reason for hiding this comment

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

Do we really need to iterate every time for each element? can't we use angle at index-1 to calculate the angle of the element at index

Choose a reason for hiding this comment

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

I think we have to iterate since the array (or function/number) indicate the angle for one item and not it's starting angle. So to get the starting angle of the current element we need to calculate it from the start angle of the chart. Or would I have missed something?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's right.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that we could compute a _starts and _angles private cache arrays in update() that contains all start angles in which case only one iteration is needed, something like:

update: function(...) {
    // ...
    var start = options.startAngle || 0;
    for (i = 0; i < count; ++i) {
        this._starts[i] = start;
        angle = [compute slice angle]; // if dataset invisible, angle == 0
        this._angles[i] = angle;
        start += angle;
    }
    // ...
    helpers.each(meta.data, function(arc, index) {
        me.updateElement(arc, index, reset);
    });
}

updateElement: function(...) {
   // ...
   var startAngle = this._starts[index];
   var endAngle = startAngle + this._angles[index];
   // ...
}

It seems to simplify a lot the computation / logic.

Choose a reason for hiding this comment

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

I hadn't thought about that but yes it could save some performance.

@etimberg
Copy link
Member

etimberg commented Oct 1, 2017

I like the idea of a scriptable option (I think we should be doing this for all new options if possible). I believe the current angle is 360 * value / sum_of_values

@benmccann benmccann added this to the Version 2.8 milestone Oct 8, 2017
@etimberg
Copy link
Member

Closing due to inactivity

@etimberg etimberg closed this Dec 31, 2017
@simonbrunel simonbrunel removed this from the Version 2.8 milestone Jan 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants