Add legend to charts with datasets

trading-peter / chart-elements

Chart.js as Polymer Elements

https://robdodson.github.io/chart-elements

267 stars 70 forks source link

Add legend to charts with datasets #24

Closed Awarua- closed 8 years ago

robdodson commented 9 years ago

I redid the chart elements so they now work closer to the original Chart.js API. If you're interested in updating this PR to work with the new structure that would be awesome

Awarua- commented 8 years ago

I am currently a little swamped with work so I am chipping away at make the legend its own behavior in my spare time. I like the changes you have made in the API.

Awarua- commented 8 years ago

@robdodson Sorry this is later than I would like, just finished all my university work for the year, and now I believe it is ready for review.

gutenye commented 8 years ago

Why not use #generateLegend() method ?

gutenye commented 8 years ago

@Awarua- There's a bug, when legend-layout="right" it becomes center align

Awarua- commented 8 years ago

@gutenye The main reason I didn't use the built in generate legend, is because I wanted full control, and it was more fun to implement the whole thing.

Regarding the layout right behavior you will have to give me more information. Currently the legend is designed so that if the legend is top or bottom relative to the graph then the legend items are laid out in a row, if the legend is left or right aligned to the graph then the legend items are meant to be in a column.

gutenye commented 8 years ago

@Awarua- I may not be clear about it, it should be this picture in a column, instead of the picture showed in my previous comment. i.e. should change align-items: center; to align-items: flex-start

Awarua- commented 8 years ago

@gutenye Ah I now see what you mean. Thanks for catching this, I believe I have fixed it in the latest commit.

gutenye commented 8 years ago

@Awarua- great :), and don't forget about my RP https://github.com/Awarua-/chart-elements/pull/1

Awarua- commented 8 years ago

@gutenye I have merged your PR :)

gutenye commented 8 years ago

@robdodson now everything is set, from my point, it's good to go. This feature is very important, when will you merge this one?

robdodson commented 8 years ago

I'm currently traveling for vacation but will try to review this when I get back next week. If chart.js already provides a legend template ability to the user, I'm a little concerned about implementing our own version of that. It's a lot of technical debt to sign up to maintain and I try to keep these elements as lightweight and close to the original API as possible. Having said that, I haven't had a chance to fully review the code so I may not be seeing the total picture just yet.

Awarua- commented 8 years ago

No rush, enjoy your vacation :smile:

Awarua- commented 8 years ago

The Chartjs 2.0 api now supports most of the legend features in this PR. So I am closing it.