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

Multiple charts in a row #97

Merged
merged 17 commits into from
Jul 13, 2018

Conversation

Prakriti-nith
Copy link
Contributor

@Prakriti-nith Prakriti-nith commented Jun 15, 2018

This feature provides the facility to visualize multiple google charts in a single cell in IRuby notebook and in a single row in web frameworks. Examples link.

@Prakriti-nith
Copy link
Contributor Author

Updated examples. I have added id as a parameter to plot object as it was required in PlotList class. I will add the tests shortly.

@Prakriti-nith
Copy link
Contributor Author

Prakriti-nith commented Jul 3, 2018

Now, multiple charts can be shown with googlecharts, google datatables, highcharts and nyaplot. Updated examples.

@coveralls
Copy link

coveralls commented Jul 6, 2018

Pull Request Test Coverage Report for Build 600

  • 108 of 113 (95.58%) changed or added relevant lines in 4 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.4%) to 96.512%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/daru/view/adapters/highcharts/display.rb 0 1 0.0%
spec/plot_list_spec.rb 60 61 98.36%
lib/daru/view/plot_list.rb 47 50 94.0%
Files with Coverage Reduction New Missed Lines %
lib/daru/view/adapters/highcharts/display.rb 4 90.91%
Totals Coverage Status
Change from base Build 578: 0.4%
Covered Lines: 2075
Relevant Lines: 2150

💛 - Coveralls

@Prakriti-nith
Copy link
Contributor Author

@Shekharrajak can you please review the changes?

Copy link
Member

@Shekharrajak Shekharrajak left a comment

Choose a reason for hiding this comment

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

Please include more than 2 charts in IRuby examples.

plot.is_a?(Daru::View::Plot) ||
plot.is_a?(Daru::View::Table)
}
raise ArgumentError, 'Invalid Argument Passed!' unless
Copy link
Member

Choose a reason for hiding this comment

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

Also write what are the valid parameter types.

@@ -4,7 +4,7 @@
require 'daru/view/adapters/googlecharts/display'

# Otherwise Daru::IRuby module was used and IRuby.html method was not working.

# Have disabled rubocop Style/Mixin for this in .rubocop.yml file
Copy link
Member

Choose a reason for hiding this comment

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

Once it is merged , then open the issue for it. So that we can keep track to solve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

@@ -17,7 +17,7 @@ AllCops:
- 'daru-view.gemspec'
- '**/*.rake'
DisplayCopNames: true
TargetRubyVersion: 2.1
TargetRubyVersion: 2.2
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated target ruby version to 2.2 in .rubocoop.yml file as rubocop was producing this error:

Error: Unsupported Ruby version 2.1 found in TargetRubyVersion parameter (in .rubocop.yml). 2.1-compatible analysis was dropped after version 0.58.

charts_script = ''
@data.each do |plot|
chart_script = extract_chart_script(plot)
charts_div_tag << chart_script.partition(%r{<div(.*?)<\/div>}ixm)[1]
Copy link
Member

Choose a reason for hiding this comment

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

It's name must be chart_id_div_tag or something that tell about the value it contains.

# TODO: Implement this for datatables too
return plot.div unless defined?(IRuby.html) &&
plot.is_a?(Daru::View::Plot) &&
plot.chart.is_a?(LazyHighCharts::HighChart)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to check HighCharts explicitly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In HighCharts, js for web frameworks (includes onload) and IRuby notebook is generated separately. So, plot.div would generate the wrong script for HighCharts in case of IRuby notebook. That's why I have checked it separately.

@@ -89,6 +88,7 @@ def show_in_iruby(placeholder=random_canvas_id)
# `high_chart_iruby` which doesn't use `onload` in chart script.
def to_html_iruby(placeholder=random_canvas_id)
# TODO : placeholder pass, in plot#div
load_dependencies('iruby')
Copy link
Member

Choose a reason for hiding this comment

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

to_html_iruby is for generating the html and javascript only. At that time will don't want to load the dependent files, right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

load_dependecies is used to load the dependencies for HighCharts modules (provided in the modules option) and HighMaps dependent files. Right now, in web frameworks too, these dependencies are loaded in to_html.

Copy link
Member

Choose a reason for hiding this comment

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

Okay in this case we have to load it during the generation of body html code.

expect(content).to match(/window.addEventListener\('load_nyaplot', render/)
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

new line here

@@ -0,0 +1,8 @@
<table class='columns'>
Copy link
Member

Choose a reason for hiding this comment

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

class='columns' is for ?

subject (:js) { combined.div }
it 'generates a table in js' do
expect(js).to match(/<table class='columns'>/)
expect(js).to match(/<td><div id=/)
Copy link
Member

Choose a reason for hiding this comment

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

check 4 times <div id= . It would be better if we can test the custom id, that was passed as well.

expect(js).to match(/<td><div id=/)
end
context 'when js of the plots is generated' do
it 'generates js of googlecharts' do
Copy link
Member

Choose a reason for hiding this comment

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

Please check the data and other tags , if required as well.

@Shekharrajak
Copy link
Member

Also commit after running IRUby examples.

@Shekharrajak Shekharrajak merged commit 2a62667 into SciRuby:master Jul 13, 2018
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.

3 participants