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

Bug #442 - Border Color #462

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Bug #442 - Border Color #462

wants to merge 6 commits into from

Conversation

codyseibert
Copy link

@codyseibert codyseibert commented Aug 12, 2016

PR fixes Bug #442

Description of change

This PR fixes the issue related to border colors not overwriting for pie and doughnut graphs. The issue was that the pie and doughnut graphs invoke the getData method which seems to have only hard coded 2 color values when there are many others. The fix was to instead look through all the keys of the colors and push them into the dataset.

Notice that I uploaded 3 newer images. Why? If you notice in the old images, the alpha for the pie and doughnut graphs appear to be 1.0, but that is not the default behavior as defined here https://github.com/jtblin/angular-chart.js/blob/master/angular-chart.js#L242. The PR fixes the bug which allows the backgroundColor to be set correctly vs before it was not.

}]
};
colors.map(function (color) {
Copy link
Owner

@jtblin jtblin Aug 16, 2016

Choose a reason for hiding this comment

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

map returns a newly created array which is not needed here, so use forEach instead.

@jtblin
Copy link
Owner

jtblin commented Aug 16, 2016

Thanks a ton for your pull request and taking the time to fix this issue! While I like that it fixes the issue, it's a bit tricky as it radically changes the color scheme for the default colors, so it's almost like a breaking change. I wonder if there is another way, is it one of the properties that is missing specifically?

@jsd219
Copy link

jsd219 commented Sep 8, 2016

Is this issue going to get fixed?

@jtblin
Copy link
Owner

jtblin commented Sep 12, 2016

@jsd219 I like that it fixes the border issue but on the other hand, it would be like a breaking change for users to change the color scheme. Also there are comments that would need to be addressed and need to be rebased. A different fix that doesn't break the color scheme would be my preference.

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.

3 participants