add option to show or hide pie chart label#133
add option to show or hide pie chart label#133peixin wants to merge 8 commits intocapitalone:masterfrom
Conversation
marzolfb
left a comment
There was a problem hiding this comment.
My apologies for the extremely delayed review.
This change breaks existing behavior in that if you haven't specified the show property on the options object, the behavior defaults to not showing labels. Also, it seems setting show to false in options doesn't actually remove the labels.
Two problems to be addressed here.
First, there is a call to fontAdapt on (new) line 100 of src/Pie.js (old line 95) that is not carrying forwarded the show property. If you look in src/util.js where fontAdapt is defined, you will see it is not returning the new show property you added. Adding that property there should take care of the problem.
Second, src/Pie.js itself has no concept of a default value if it doesn't find the show property. Existing users of the library using Pie chart wouldn't have this property set in their options. So, inclusion of this PR would unexpectedly make the pie chart labels disappear.
|
One additional note is that it looks like this change breaks the jest tests that were in place for the Pie chart (one of the few chart types that have any tests for it). Can you also make sure |
Thank you for contributing a pull request.
Please ensure that you have signed the CLA.