airblade / chartjs-ror

[Not supported - see #53] Simplifies using Chart.js in Rails
MIT License
184 stars 57 forks source link

Update to ChartJS 2.4.0 and some improvements #33

Closed sbosio closed 7 years ago

sbosio commented 7 years ago
airblade commented 7 years ago

Thanks, I hope to find time to look at this properly tomorrow.

ignisf commented 7 years ago

Hello @airblade, does this change look good for merging to you?

airblade commented 7 years ago

@ignisf Thanks for the reminder! This had dropped off my radar.

@sbosio Where did your to_javascript_string() come from? It looks sensible but it would be nice to be able to have a bit more confidence.

sbosio commented 7 years ago

@airblade, I wrote that to_javascript_string method. I'm pretty sure it will work well in all cases, because JSON notation isn't too complicated. The problem, as you already know, arises with the differences between a JSON object and a JavaScript object which is what ChartJS expects to initialize the chart. You were using the JSON objects directly like JS objects, but JSON can't handle methods as values, so when a Ruby string contained a function definition, you removed the surrounding double-quotes to "convert" the JSON to a JS object (as all other keys and values are correctly interpreted). The problem is that converting the string to JSON adds some escape sequences when the function itself contains chars like double-quotes or backslashes making the function sintactically incorrect. I just realized the to_javascript_string isn't handling some cases. One of them is the 'nil' case, when used as a value, other could be dates and times. Perhaps it will be useful to change the element.to_s after the else inside the case statement to element.to_json.

airblade commented 7 years ago

@sbosio Thanks for the explanation.

I can't help but feel this is the sort of thing that should have a standard solution, but I can't find one.

Anyway:

Then we always end up with JavaScript-safe output. Since Ruby defines to_json on most things we don't have to worry about dates or nil or big decimals etc.

sbosio commented 7 years ago

@airblade yes, there's no standard solution for this. I have faced the same problem with DataTables, you can initialize it with HTML5 'data' properties, but you can't do so for options that expect a method, you have to do it in plain JS. Both of your proposed to_json changes seem to be correct. On keys it shouldn't be necessary because there aren't any ChartJS options that require escaping. AFAIK, they're all plain ASCII, but still it will be safer to use it if someone mistakenly writes a key string with chars that need escaping. In the final else, as you noticed, the element.to_s has to be changed to element.to_json so it handles all other types correctly, including nil. There's a thing with BigDecimal, as it gets encoded as a JSON string (i.e. with double-quotes surrounding the value), I don't know why. Perhaps it should be necessary to handle that situation separately on a when element.is_a?(BigDecimal) and use element.to_s instead of element.to_json.

airblade commented 7 years ago

Your change (10ab2e3) is in v3.3.1. Thanks again.