Skip to content
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

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

agrass
Copy link

@agrass agrass commented Apr 14, 2015

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

@gordonwoodhull
Copy link
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
Copy link
Author

agrass commented Apr 14, 2015

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
Copy link
Author

agrass commented Apr 18, 2015

@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

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

@gordonwoodhull
Copy link
Contributor

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

@renoth
Copy link
Contributor

renoth commented Jul 27, 2015

+1

@agrass
Copy link
Author

agrass commented Aug 5, 2015

@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