Skip to content
This repository has been archived by the owner on Sep 17, 2021. It is now read-only.

add option to show or hide pie chart label #133

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

Conversation

peixin
Copy link

@peixin peixin commented Mar 28, 2017

Thank you for contributing a pull request.

Please ensure that you have signed the CLA.

  • I have signed the CLA

Copy link
Contributor

@marzolfb marzolfb left a comment

Choose a reason for hiding this comment

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

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.

@marzolfb
Copy link
Contributor

marzolfb commented Apr 9, 2017

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 npm test succeeds after making your changes? Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants