Legend grouping #43
Legend grouping #43
Conversation
|
@SpaceK33z is there an issue with this PR on your side or is there a reason this is open? If not, I'd review and merge it. |
|
@tarekis, nope no issue, looks like a very good PR with examples+tests. Just didn't have the time to properly review. |
|
@SpaceK33z sweet, thanks for the heads up. |
|
Sure! |
|
@guimeira Sorry for the huge delay, I've been seriously sick and wasn't available at all, I'll review it this week! 🙂 |
| @@ -70,6 +70,20 @@ | |||
| background-color: #453d3f; | |||
| border-color: #453d3f; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Please add all changes to the demo page to the https://github.com/CodeYellowBV/chartist-plugin-legend/tree/gh-pages branch, i will probably rearrange this in the future, but for now all changes need to be done on this branch.
chartist-plugin-legend.js
Outdated
| @@ -57,6 +53,25 @@ | |||
| } | |||
|
|
|||
| return function legend(chart) { | |||
| function updateChart() { | |||
There was a problem hiding this comment.
That function doesn't really makes sense, does it? It gets called only once, and has local variables referenced it.
|
Sorry for the huge delay! I hope you are not sick anymore :) I removed the I also created #52 adding the documentation to the Please let me know if there is anything else I should change 😃 |
|
LGTM! Thanks for the contribution! Will look into deploying on npm later today, gotta wait a bit for that, can't do it from work really. |
I've finally found some time to write the tests I promised on issue #38 😄
I basically did all the tests of the
clickablepart using the legend grouping and making sure the series are shown and hidden accordingly. I think that should be enough since the clicking behavior is what this new feature changes (and all the other tests are passing, so all the other features apparently are not affected by these changes), but if you think something else should be covered, please let me know and I'll write some more tests.