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

allow decoupling of chart name and class name #40

Merged
merged 1 commit into from
Oct 16, 2012
Merged

allow decoupling of chart name and class name #40

merged 1 commit into from
Oct 16, 2012

Conversation

greinacker
Copy link
Contributor

From my comment in issue #38 -

I wanted to override individual chart types (e.g. GoogleVisualr::Interactive::BarChart) to carry some extra data around before rendering. In my case I made a "ClickableBarChart". This also required redefining BaseChart.to_js, because it uses class_name to specify the Google chart type. I propose we have something like "chart_name" as a property, and in BaseChart that simply returns class_name...but in subclasses this could be overridden to be whatever was necessary.

The attached code implements this...

@travisbot
Copy link

This pull request passes (merged 8284f11 into 13c76cb).

@greinacker
Copy link
Contributor Author

Sample code using this:

class ClickableBarChart < GoogleVisualr::Interactive::BarChart

  def chart_name
    "BarChart"
  end

  # other fancy custom clickable bar chart code here
end

@winston
Copy link
Owner

winston commented Oct 15, 2012

Hi, sorry for taking such a long time to get back!

Anyway, the change looks good. But I am thinking of putting this chart_name method in packages.rb instead.

Will that still work for your code? I believe it should. Let me know.

Thank you!

@greinacker
Copy link
Contributor Author

Seems to me moving it to packages.rb should be fine.

winston added a commit that referenced this pull request Oct 16, 2012
@winston winston merged commit 8284f11 into winston:master Oct 16, 2012
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

3 participants