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

Issue 1764: Add default colors for Pie Charts #1834

Merged
merged 7 commits into from
Dec 27, 2015
Merged

Issue 1764: Add default colors for Pie Charts #1834

merged 7 commits into from
Dec 27, 2015

Conversation

ds17f
Copy link
Contributor

@ds17f ds17f commented Dec 26, 2015

I wrote a simple fix for default colors on the pie chart.

Here's the basic idea:

  1. Create 2 new arrays in the global.defaults
    • segmentColorDefaults
    • segmentHighlightColorDefaults
  2. Populate each of these arrays with a list of colors (ie: #AABBCC)
  3. Update the "addData" routine on the Pie Chart so that, if a color is not specified, a color and highlight color is chosen from the defaults
    • For simplicity, the index of the item which is being added is used to look up the corresponding color in the list of default colors
      • I suppose I could use a stack of some sort and pop off the top each time a color is used, but that's more than I'm up for at this moment

Users can then override the list of default colors/highlights at a global level should they feel the predefined ones are not adequate/enough. Failure to define colors will simply roll over to the defaults.

@@ -92,6 +92,10 @@
},
addData : function(segment, atIndex, silent){
var index = atIndex !== undefined ? atIndex : this.segments.length;
if ( typeof(segment.color) === "undefined" ) {
segment.color = Chart.defaults.global.segmentColorDefault[index];
Copy link
Member

Choose a reason for hiding this comment

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

Might be safer to use index % Chart.defaults.global.segmentColorDefault.length
It is unlikely that there would be more than 15 segments, but someone could change the array to a shorter one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely a good call. I added a commit to revise this.

@etimberg
Copy link
Member

Overall looks pretty good. Did you see https://github.com/nnnick/Chart.js/blob/master/src/Chart.Doughnut.js#L76 as well?

@ds17f
Copy link
Contributor Author

ds17f commented Dec 26, 2015

@etimberg I hadn't seen that. I'm not sure I get exactly what that's doing without context and I'm not sure if it needs to be patched to support this change.

Forgive me, JS isn't my primary language. I'm happy to finish out the change but I need a little guidance if you would.

Thanks

@ds17f
Copy link
Contributor Author

ds17f commented Dec 26, 2015

I'm going to take a look at the default color set too. I kind of just picked a random set but I'm sure I could choose a better set of colors/highlights that make them go better together.

@etimberg
Copy link
Member

I don't think it needs to be patched to support it. It simply adds a random colour if one is not added. Let me know when you've settled on the final colour set and I'll merge this. Thanks for the PR 😄

@etimberg etimberg added this to the Version 1.1 milestone Dec 26, 2015
@ds17f
Copy link
Contributor Author

ds17f commented Dec 27, 2015

@etimberg I chose to use 12 colors instead of the 15(?) I had before. I read somewhere that that's the most an eye can process. I also tried to pick good main/highlight colors. It looks good to me. You can merge when you feel it passes muster.

You're welcome for the PR. Thanks for the excellent project! It is the tool that I needed to make my work beautiful.

@etimberg
Copy link
Member

Tested, looks nice! Great colour choices!

etimberg added a commit that referenced this pull request Dec 27, 2015
…fault-pie-chart-colors

Issue 1764: Add default colors for Pie Charts
@etimberg etimberg merged commit 92d4d98 into chartjs:master Dec 27, 2015
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.

2 participants