dc.js icon indicating copy to clipboard operation
dc.js copied to clipboard

add legendLabelAccessor to piechart

Open agrass opened this issue 9 years ago • 7 comments

I added a method legendLabelAccessor to change the text on legends in piecharts. It's the same of issue #785

agrass avatar Apr 14 '15 22:04 agrass

Hi @agrass, this will be a useful contribution, and the implementation looks straightforward.

A few questions:

  • I am curious what you need getLegendLabel for? The other accessors don't pass the chart as this, so maybe this helper function isn't necessary.
  • Would you be willing two simple tests that the text is applied correctly when the function is used, and is just the key when the function is not used? There is a helper function legendLabel in spec/legend-spec.js that would be helpful.
  • Would you be willing to add a line of documentation?

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!

https://github.com/dc-js/dc.js/edit/develop/CONTRIBUTING.md

gordonwoodhull avatar Apr 14 '15 22:04 gordonwoodhull

Hi @gordonwoodhull, thank you for your fast reply!

Yes the method getLegendLabel isn't helping too much, I'm going to remove that method and call the legendLabelAccessor directly. About the test and documentation you are right, it was a first approach so now I'm going to complete.

Thanks

agrass avatar Apr 14 '15 22:04 agrass

@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 legendLabelAccessor. What do you think? Please let me know if you think that another change is needed. I added the method on basemixin, but for the while I dont know in which another chart may apply the same (but now it will be more easy add that method in case of needed it)

agrass avatar Apr 18 '15 20:04 agrass

This would be great to have! What's the status of getting it in?

tlrobinson avatar Jul 08 '15 21:07 tlrobinson

LGTM; I think all we need to do is also use the accessor in the other "legendable" charts.

gordonwoodhull avatar Jul 08 '15 22:07 gordonwoodhull

+1

renoth avatar Jul 27 '15 08:07 renoth

@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;
    })

agrass avatar Aug 05 '15 20:08 agrass