Fix #22 and add docs #25
Conversation
…, which is no longer given
| li.setAttribute('data-legend', i); | ||
| li.textContent = legend.name || legend; | ||
| legendElement.appendChild(li); | ||
| for (var legendNamesIndex = 0, length = legendNames.length; legendNamesIndex < length; legendNamesIndex++) { |
There was a problem hiding this comment.
Why did you rewrite forEach to for?
There was a problem hiding this comment.
Performance is way higher, and forEach is plain syntax sugar. It does lots of unneeded checks on each iteration.
There was a problem hiding this comment.
If jsperf wasn't down you could see the reference test here. Let's hope jsperf v2 comes soon...
There was a problem hiding this comment.
I know about that test, but this is micro-optimization, and doesn't actually make a difference. I consider the advantages of forEach to be more important; readability, less chance of a off-by-one error, and having a separate scope. If it would be about thousands of items in the array I would consider using for, but for this not. Sorry.
|
Hi, thanks for this. It's a huge PR, so it will take some time to review. |
|
Sure thing, thanks for your time to review it! |
| }) | ||
| ] | ||
| });</code></pre> | ||
| <h3>Chart with <i>clickable</i>:</h3> |
There was a problem hiding this comment.
The default value is clickable: true. It says here that the legend should be clickable, but you pass clickable: false, making it not-clickable ;).
There was a problem hiding this comment.
Yeah, i actually changed the value to false, so actual changes for the option can be seen on the page; it would be quite irritating not to change anything, this way the viewer won't see any changes the option makes, but the description is indeed misleading. I'd change the description to:
Sets the legends clickable state; setting this value to false disables toggling graphs on legend click.
Please let me know if you can agree with that making sense.
There was a problem hiding this comment.
Allright, yeah it was only the text that confused me. That change would be fine.
|
Other then the changes above, it looks good. The example page is really gorgeous now :). |
|
For a next time, it would be better to make a separate PR for each feature. It makes changes easier to review and reason about. I won't ask you to do it for this one, because that would probably be a huge pain. |
|
You're right twice, it would be a huge pain, and i realized my error the moment i pushed the commits; gonna seperate future PR's ;) |
|
Reverted the enumeration and loop conversion and fixed the |
|
Great. Thanks again! |
|
Ehmm whoops |
|
Published in |
|
No problem! Also, i just realized that the |
|
No it's fine, I'll also update the |
Adds option
position, which is enumerated and accepts one of two positions currently: 'bottom' and 'top'. Now the legend element will (correctly) be added to the chart container on thecreatedevent, and pre- or appended to thesvg.I also reworked the demo page, added a small documentation to it, and rephrased option descriptions to be more precise.