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.

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

Image charts #26

Merged
merged 10 commits into from
Mar 1, 2012
Merged

Image charts #26

merged 10 commits into from
Mar 1, 2012

Conversation

raphaelcm
Copy link
Contributor

Adds image charts and the #uri method, which returns a URI for getting a static chart image through Google Charts API. Includes specs. See http://code.google.com/apis/chart/image/docs/making_charts.html for details.

e.g.:

GoogleVisualr::Image::PieChart.new( data_table, options ).uri
  => #<URI::HTTPS:0xa958134 URL:https://chart.googleapis.com/chart?...>

My previous pull request (#25) was incomplete, please ignore and review this one.

@winston
Copy link
Owner

winston commented Jan 19, 2012

Hi,

Thanks for the pull request, but I am a little confused. Are you adding support for:
http://code.google.com/apis/chart/interactive/docs/gallery/imagepiechart.html
or
http://code.google.com/apis/chart/image/docs/gallery/pie_charts.html

They are two different type of charts:

  • the 1st is part of the "Interactive Charts", and a natural extension of the lib because it uses DataTable
  • the 2nd is part of the "Image Charts", which creates a chart through building out a URL

The lib's current focus is on the "Interactive Charts" component, and not the "Image Charts" component.

Thanks!

@raphaelcm
Copy link
Contributor Author

I'm adding support for the image charts. I realize it's not the focus, but a lot of the code can be reused to ease creation of the chart image URLs, which are necessary in contexts where JS execution is not possible (e.g. emails generated by ActionMailer).

@raphaelcm
Copy link
Contributor Author

Also, image charts can be build via JS, and are part of the JS API: http://code.google.com/apis/chart/interactive/docs/more_charts.html (scroll to bottom).

This patch would enable one to put charts in a metrics page, and put the same charts in an email (where JS can't be executed) using the #uri method. This is the exact use case I have where I work, which is why I created the patch.

@winston
Copy link
Owner

winston commented Jan 19, 2012

That's a valid use case, but the image portion in the lib was "reserved" for Interactive's image charts, and changing it to support the pure image charts seems a little hacky to me at the moment.

I'll need to look at the code in detail to see if there's a better way to integrate and support both kind of charts.

@raphaelcm
Copy link
Contributor Author

I guess I misspoke...it supports the "interactive" image charts, but adds the #uri method to map their options to query string parameters.

@winston
Copy link
Owner

winston commented Jan 19, 2012

Ah ok. I see how the JS translates to the actual images now.

I'll definitely see how I can merge this in. Thanks!

@winston
Copy link
Owner

winston commented Jan 21, 2012

I pulled the code this afternoon and had a look at it.

It looks pretty good, but I think it requires a little bit more work to make it
entirely compatible with Google's chart configuration options.

For example, to create an image pie chart, I would look at http://code.google.com/apis/chart/interactive/docs/gallery/imagepiechart.html,
specify any of the configuration options (as described in the "Configuration Options" section)
and pass it along during the initialization of an image chart object.

Then I would have the option of either rendering this in the view as js, or as an image (with the new #uri call).
And in both cases, the charts should look the same.

However, I noticed that some of the configuration options are not translated to Google Image Chart parameters.
E.g. legend, backgroundColor.

And in the #chart_image_url method, there are translations for titleTextStyle and legendTextStyle,
but which were not listed in the configuration options.

As much as possible, I think we should try and translate all the configuration options to Google Image Chart parameters,
and omit those that are not listed.

This makes the gem much more robust and reflective of the API as described by Google.

Let me know what you think? Thanks!

@raphaelcm
Copy link
Contributor Author

Agreed. I've just created a google spreadsheet mapping the image chart configuration options to query string parameters (http://code.google.com/apis/chart/image/docs/chart_params.html#top). Take a look here: https://docs.google.com/spreadsheet/ccc?key=0AsV-ZyoknLVXdDg4bTBodnJwQnNZRjluSHlOVFhjNUE

I'll update the code so that, as you recommend, any configuration option that can be mapped to a query string parameter, will be. For configuration options that don't map, users can pass in their own options via the #uri method, e.g.:

some_chart.uri(:chdls => "0000CC,14")

Sound good? Will get started now but with my work schedule may take a few days to finish.

@raphaelcm
Copy link
Contributor Author

Check out the latest commit and see what you think. I also color coded all the configuration options in the google doc to reflect how well I was able to map them to query string parameters.

https://docs.google.com/spreadsheet/ccc?key=0AsV-ZyoknLVXdDg4bTBodnJwQnNZRjluSHlOVFhjNUE

@raphaelcm
Copy link
Contributor Author

Hi, just wanted to check in about this. My company's been using this patch in production for a while and are pretty happy with it. I've just committed a couple fixes for minor bugs we came across, but nothing major.

Let me know what you think and if there are any other improvements you'd like to see.

Thanks!

winston added a commit that referenced this pull request Mar 1, 2012
Add image charts to image charts.
@winston winston merged commit 0b31855 into winston:master Mar 1, 2012
@winston
Copy link
Owner

winston commented Mar 1, 2012

Sorry for the delay. Looks good. Merged, and bumping up the gem version in a bit. Thanks!

@raphaelcm
Copy link
Contributor Author

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants