-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 legendLabelAccessor to piechart #916
base: develop
Are you sure you want to change the base?
Conversation
Hi @agrass, this will be a useful contribution, and the implementation looks straightforward. A few questions:
In the bigger picture, I also wonder if the functionality belongs higher up, like in baseMixin. It seems like this must be an issue for other kinds of charts. I'll comment on the issue. Thanks! |
Hi @gordonwoodhull, thank you for your fast reply! Yes the method Thanks |
@gordonwoodhull I added some test and documentation. I checked on piechart specs, and there are already a test who checks if the text of the legend is the same text of the key group, so I added another test to check if the text change when I use the |
This would be great to have! What's the status of getting it in? |
LGTM; I think all we need to do is also use the accessor in the other "legendable" charts. |
+1 |
@gordonwoodhull does it make sense for another "legendables" charts? Because when you set the legend label for a bar or line chart, the name apply for the whole group, so is a little different compared with piechart that each key of the group has a different label. You can set the legend label without an acessor: .group(frequencies, "Apples").valueAccessor(function (d) {
return d.value.apples;
})
.stack(frequencies, "Kiwis", function (d) {
return d.value.kiwis;
}) |
I added a method legendLabelAccessor to change the text on legends in piecharts. It's the same of issue #785