Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support generating chart Javascript without <script> tag #69

Merged
merged 2 commits into from
Apr 5, 2014
Merged

Support generating chart Javascript without <script> tag #69

merged 2 commits into from
Apr 5, 2014

Conversation

k1w1
Copy link
Contributor

@k1w1 k1w1 commented Mar 27, 2014

This allows JS to be used inside a callback function, e.g. if the Google jsapi code is being loaded dynamically rather than in <head> you need code like:

$.getScript('https://www.google.com/jsapi', function (data, textStatus) {
  <%= render_chart(@status_chart, 'status-chart', :script_tag => false) %>
});

@winston
Copy link
Owner

winston commented Apr 5, 2014

Hi! Thanks for the pull request! In principle, I am fine with this change, and I'll merge this in first.

However, I have been thinking about this ajax/dynamic load problem for a while (and there are often issues about this as well), and I was wondering if this would be better for the long run:

  • in base_chart.rb, I'll split the to_js into 3 methods
    • to_js which wraps the chart_js and load_js with script tag
    • chart_js which only contains the draw chart function
    • load_js which contains the line to load the package and call the draw chart function
  • and then expose these methods as view helpers in Rails so it can used as in your use case and for other ajax/dynamic loads.

I'll probably work on this next week and see if I can come with something good.

Thank you!

winston added a commit that referenced this pull request Apr 5, 2014
@winston
Merge pull request #69 from k1w1/master
5a36423 
Support generating chart Javascript without <script> tag
@winston winston merged commit 5a36423 into winston:master Apr 5, 2014
@k1w1
Copy link
Contributor Author

k1w1 commented Apr 5, 2014

i agree with your proposal - I think it is a better long term solution.

@winston
Copy link
Owner

winston commented Apr 8, 2014

I made some modifications as per above and pushed a new version for the gem.

Decided to let the render_chart method signature stay the same with options passed in,
so you shouldn't see any difference.

Only did the changes to split base_chart.to_js into 3 methods for use on their own.

Let me know if the new version breaks for you. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@k1w1 @winston