Skip to content

add legendLabelAccessor to piechart #916

Open
agrass wants to merge 12 commits into
dc-js:developfrom
agrass:addLegendLabelAccessor
Open

add legendLabelAccessor to piechart #916
agrass wants to merge 12 commits into
dc-js:developfrom
agrass:addLegendLabelAccessor

Conversation

@agrass

@agrass agrass commented Apr 14, 2015

Copy link
Copy Markdown

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

@gordonwoodhull

Copy link
Copy Markdown
Contributor

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

@agrass

agrass commented Apr 14, 2015

Copy link
Copy Markdown
Author

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

agrass commented Apr 18, 2015

Copy link
Copy Markdown
Author

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

@gordonwoodhull gordonwoodhull added this to the v2.1 milestone May 24, 2015
@tlrobinson

Copy link
Copy Markdown

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

@gordonwoodhull

Copy link
Copy Markdown
Contributor

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

@renoth

renoth commented Jul 27, 2015

Copy link
Copy Markdown
Contributor

+1

@agrass

agrass commented Aug 5, 2015

Copy link
Copy Markdown
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants