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

Add embeddable charts #268

Closed
wants to merge 1 commit into from
Closed

Add embeddable charts #268

wants to merge 1 commit into from

Conversation

jules2689
Copy link
Contributor

@jules2689 jules2689 commented Nov 7, 2019

What this does

This adds the option to embed a chart outside of the blazer engine.

<%= render "blazer/queries/query", query: query %> will render the chart and all relevant pieces (JS, Views, Helpers, etc).

params can be provided, just like in a dashboard, to allow variables to be passed in. The API is the same

Combined with ahoy, I was able to embed this in my app:
image

I tried to make this as seamless as possible, and the Blazer::Query activerecord call + render call is the simplest API I could think of 😄

Addresses part of #24 (#24 (comment))

Notes

  • I chose not to use the _query.html.erb file in the dashboard (from where I took much of the view code) as I intended to also embed some JS and stylesheet tags.
  • While the global var check in the view is a little iffy, I felt it was better than including JS/CSS many times. I also thought it was nicer than asking the user to include those in their application view code.
  • I extracted the relevant CSS classes to a chart.css file so we can load that separately.
  • Open to changing any of these decisions, of course 😄

This adds the option to embed a chart outside of the blazer engine.
<%= render "blazer/queries/query", query: query %> will render the chart and all relevant pieces.

Params can be provided, just like in a dashboard, to allow variables to be passed in. The API is the same
@@ -3,9 +3,14 @@ class Engine < ::Rails::Engine
isolate_namespace Blazer

initializer "blazer" do |app|
ActiveSupport.on_load :action_view do
include Blazer::BaseHelper
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required so we can call blazer_js_var.
I originally has this in my app, but I didn't like the feeling of this extra step. I wanted to get to a simple API of just a render call + activerecord call.

I don't think this is a concern, especially as we have prefixed all methods with blazer_.

Can anyone think of trouble this could cause?

}, function (message) {
$("#chart-<%= query.id %>").addClass("query-error").html(message)
});
</script>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from https://github.com/ankane/blazer/blob/master/app/views/blazer/dashboards/show.html.erb#L35-L50

I chose not to use this fragment there as I also wanted to embed some stylesheet_link_tag and javascript_include_tag and we had a call to rootPath as well.

@ankane
Copy link
Owner

ankane commented Nov 8, 2019

Hey @jules2689, thanks for the PR 👍 This is a neat idea, but it's outside the scope of what I'd like to support right now.

@jules2689
Copy link
Contributor Author

Hey @ankane, Many thanks for this gem! 💎

No worries.

Are there any thoughts you can share about why this is outside of that scope, or a doc I can read? 😄

As an aside, #24 is open and suggests that this is a desirable feature. Might I suggest closing or updating that issue to avoid future confusion? 🙇

@ankane
Copy link
Owner

ankane commented Nov 8, 2019

Re scope: I think it's simplest for Blazer to be treated as a separate product (that just happens to be mounted on your app), so Rails-specific features are generally discouraged.

Re #24: Sorry about the confusion - I've updated it.

@jules2689
Copy link
Contributor Author

Makes sense, I don't think this would be difficult to make work from inside the app itself.
I'll report back with a guide on that, if I do :)

@jules2689
Copy link
Contributor Author

jules2689 commented Nov 8, 2019

Got it to work!
https://gist.github.com/jules2689/e0a90e0c58cea52969e531a60d0915e4

One thing that would be nice is if there was a single JS file and single CSS file to include.
e.g.
assets/chart-bundle.js included

  • blazer/moment
  • blazer/moment-timezone-with-data
  • blazer/Chart.js
  • blazer/chartkick
  • blazer/stupidtable
  • blazer/stupidtable-custom-settings
  • blazer/queries
  • blazer/routes

And assets/charts.css included the CSS from this PR.

Thoughts on that @ankane? It would make this much neater and could help blazer by separating the JS and CSS to concerns?

@ankane
Copy link
Owner

ankane commented Nov 8, 2019

Great, thanks for sharing. Re files: you should be able to add that to the instructions ("create app/assets/chart-bundle.js with ...). I don't think it makes sense to make internal changes for unsupported features.

@jules2689
Copy link
Contributor Author

e files: you should be able to add that to the instructions ("create app/assets/chart-bundle.js with ...).

Done.

Closing this, people searching can find the link and see the gist here: https://gist.github.com/jules2689/e0a90e0c58cea52969e531a60d0915e4

@jules2689 jules2689 closed this Nov 11, 2019
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.

2 participants