Wordpress vip code review patch #1
Conversation
…arArea legendCallback
Update documentation for generateLegend method; fix corresponding unit tests
…ds compatibility. Reverts test updates
src/plugins/plugin.legend.js
Outdated
| var list = document.createElement('ul'); | ||
| list.setAttribute('class', chart.id + '-legend'); | ||
|
|
||
| if (datasets.length) { |
There was a problem hiding this comment.
I'd probably remove this and instead handle it in the for-loop. Also, it's more inline with the project's coding standards to declare all variables at the top of the method. Overall this PR looks good to me though
var list = document.createElement('ul');
var i, ilen, listItem, listItemSpan;
list.setAttribute('class', chart.id + '-legend');
for (i = 0, ilen = datasets.length || 0; i < ilen; i++) {
There was a problem hiding this comment.
Note the same comment would apply to the other two files as well
There was a problem hiding this comment.
Hey @benmccan, thanks for your feedback on this. I've made additional code review fixes which you can preview in this commit: 1a01415
Please let me know if there's anything else I can do to help progress these changes. For example, I don't seem to be able to re-open the original PR into the chartjs repository, which I think is necessary for these changes to show up there.
Thank you again for your help here.
Guidelines for contributing: https://github.com/chartjs/Chart.js/blob/master/docs/developers/contributing.md