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

Added HighMaps #92

Merged
merged 18 commits into from
Jun 16, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions lib/daru/view/adapters/highcharts.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ module HighchartsAdapter
# @param [Array/Daru::DataFrame/Daru::Vector] data
#
def init(data=[], options={})
data_new = guess_data(data)
# TODO : for multiple series need some modification
# Alternate way is using `add_series` method.
#
# There are many options present in Highcharts so it is better to use
Expand All @@ -42,10 +40,17 @@ def init(data=[], options={})
# all the options present in `options` and about the
# series (means name, type, data) used in f.series(..)
f.options = options.empty? ? LazyHighCharts::HighChart.new.defaults_options : options

series_type = options[:type] unless options[:type].nil?
series_name = options[:name] unless options[:name].nil?
f.series(type: series_type, name: series_name, data: data_new)
# For multiple series when data is in a series format as in
# HighCharts official examples
# TODO: Add support for multiple series when data as Daru::DataFrame/Daru::Vector
if data.is_a?(Array) && data[0].is_a?(Hash)
Copy link
Member

Choose a reason for hiding this comment

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

this if data.is_a?(Array) && data[0].is_a?(Hash); must be under guess_data , right ?

Please share the link for multiple series example.

Copy link
Contributor Author

@Prakriti-nith Prakriti-nith Jun 3, 2018

Choose a reason for hiding this comment

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

Multiple series are used in almost all the HighCharts and HighMaps examples like: (link1, link2, link3). guess_data does not work for Array of Hashes (Multiple series) and will raise ArgumentError. Our data is already in the series format and can be used directly.
As highcharts supports multiple series in a single chart, I was thinking that in future a feature where multiple Daru::DataFrame, Daru::Vector can be used in a single Highchart/stock/map can be added.

Copy link
Member

Choose a reason for hiding this comment

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

It is already working for multiple series, since it is checking for array in guess_data , 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.

Yes, it is checking but not working. As here #chart.series method is generating a single series with single data parameter. Multiple series contain Array of hashes and data parameter for each hash (i.e. for each series).
Also, I tried using #chart.series_data for data_new as well but this works only when data is provided as in official highcharts series format (i.e. Array of Hashes).

f.series_data = data
else
data_new = guess_data(data)
series_type = options[:type] unless options[:type].nil?
series_name = options[:name] unless options[:name].nil?
f.series(type: series_type, name: series_name, data: data_new)
end
end
@chart
end
Expand Down
61 changes: 56 additions & 5 deletions lib/daru/view/adapters/highcharts/display.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
require_relative 'layout_helper_iruby'
require_relative 'iruby_notebook'
require 'daru/view/constants'

module LazyHighCharts
def self.init_script(
dependent_js=['highstock.js', 'highcharts-more.js', 'modules/exporting.js',
'highcharts-3d.js', 'modules/data.js']
dependent_js=HIGHCHARTS_DEPENDENCIES
)
# Highstock is based on Highcharts, meaning it has all the core
# functionality of Highcharts, plus some additional features. So
Expand Down Expand Up @@ -33,22 +33,25 @@ class HighChart
#
def to_html(placeholder=random_canvas_id)
chart_hash_must_be_present
script = load_dependencies('web_frameworks')
Copy link
Member

Choose a reason for hiding this comment

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

So now each time dependencies will be loaded, whenever to_html is invoked. Can we control this loading (it will load once), using Proxy design pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to_html will load the dependencies only if there is some additional dependency provided in the modules option or if a map is being drawn (to load the required map data). Otherwise, load_dependencies will simply return an empty string. I will look into the Proxy design pattern also.

# Helps to denote either of the three classes.
chart_class = extract_chart_class
# When user wants to plot a HighMap
if chart_class == 'Map'
high_map(placeholder, self)
script << high_map(placeholder, self)
# When user wants to plot a HighStock
elsif chart_class == 'StockChart'
high_stock(placeholder, self)
script << high_stock(placeholder, self)
# When user wants to plot a HighChart
elsif chart_class == 'Chart'
high_chart(placeholder, self)
script << high_chart(placeholder, self)
end
script
end

def show_in_iruby(placeholder=random_canvas_id)
# TODO : placeholder pass, in plot#div
load_dependencies('iruby')
IRuby.html to_html_iruby(placeholder)
end

Expand All @@ -61,6 +64,54 @@ def to_html_iruby(placeholder=random_canvas_id)
high_chart_iruby(extract_chart_class, placeholder, self)
end

# Loads the dependent mapdata and dependent modules of the chart
#
# @param [String] to determine whether to load modules in IRuby or web
# frameworks
# @return [void, String] loads the initial script of the modules for IRuby
# notebook and returns initial script of the modules for web frameworks
def load_dependencies(type)
dep_js = extract_dependencies
if type == 'iruby'
LazyHighCharts.init_iruby(dep_js) unless dep_js.nil?
elsif type == 'web_frameworks'
dep_js.nil? ? '' : LazyHighCharts.init_script(dep_js)
end
end

# Extracts the required dependencies for the chart. User does not need
# to provide any mapdata requirement explicity in the `options`.
# MapData will be extracted using `options[:chart][:map]` already
# provided by the user. In `modules` user needs to provide the required
# modules (like tilemap in highcharts) in the form of Array. Once the
# dependency is loaded on a page, there is no need to provide it again in
# the `modules` option.
#
# @return [Array] the required dependencies (mapdata or modules)
# to load the chart
def extract_dependencies
dep_js = []
# Mapdata dependencies
get_map_data_dependencies(dep_js)
# Dependencies provided in modules option (of highcharts mainly
# like tilemap) by the user
dep_js |= options.delete(:modules).map! { |js| "#{js}.js" } unless
Copy link
Member

Choose a reason for hiding this comment

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

I think , options created by us (like chart_class, modules, map ) must be in separate parameter. Only highcharts options must be passed as 2nd parameter. WDYT?

Copy link
Contributor Author

@Prakriti-nith Prakriti-nith May 30, 2018

Choose a reason for hiding this comment

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

map option is not created by us and will be provided in highcharts' options everytime there is a need for map data(like europe.js). This would be the exact address of the dependency required.
For example, a user using dependency https://code.highcharts.com/mapdata/countries/in/in-all.js will provide countries/in/in-all as a value for options[:chart][:map].
Only the options chart_class and modules are created by us. User does not need to provide any additional option for mapdata.
Yes, we can have a third parameter for these but IMO that would be only more complex for the user plus we then have to generalize it for other adapters as well which I think is not a good option.

Copy link
Member

Choose a reason for hiding this comment

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

User will put path for the js to load in modules options (indirectly), right? Any other use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modules option is created just to load some additional dependencies used in a specific chart by the user.

options[:modules].nil?
dep_js
end

# @param dep_js [Array] JS dependencies required for drawing a map(mapdata)
# @return [void] Appends the map data in dep_js
# rubocop:disable Metrics/AbcSize
def get_map_data_dependencies(dep_js)
Copy link
Member

Choose a reason for hiding this comment

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

According to HighMap option map, we are loading the respective js files (so no need to load not used files).

But need to find a way to capture the loaded js files, so that next time we run to_html or generate the script it won't load the file, that is already loaded into the same web-page.

dep_js.push('mapdata/' + options[:chart][:map].to_s + '.js') if
Copy link
Member

Choose a reason for hiding this comment

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

if (condition)
  .. statements... 
  ....

is easy to understand rather than this.

!options[:chart_class].nil? &&
options[:chart_class].capitalize == 'Map' &&
options[:chart] &&
options[:chart][:map]
end
# rubocop:enable Metrics/AbcSize
Copy link
Member

Choose a reason for hiding this comment

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

Did you try to fix rubocop error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried removing it but the conditions are necessary. Please provide any suggestions to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

I guess, it must have method to check for map chart and chart class. if condition inside if condition looks bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it earlier but still, there was rubocop error. I have separated out the method.

Copy link
Member

Choose a reason for hiding this comment

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

remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO all the four conditions are necessary. Which one should I remove?


# @return [String] the class of the chart
def extract_chart_class
# Provided by user and can take two values ('stock' or 'map').
Expand Down
8 changes: 6 additions & 2 deletions lib/daru/view/adapters/highcharts/iruby_notebook.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'daru/view/constants'

module LazyHighCharts
# generate initializing code
def self.generate_init_code(dependent_js)
Expand All @@ -9,15 +11,17 @@ def self.generate_init_code(dependent_js)

# Enable to show plots on IRuby notebook
def self.init_iruby(
dependent_js=['highstock.js', 'highcharts-more.js', 'modules/exporting.js',
'highcharts-3d.js', 'modules/data.js']
dependent_js=HIGHCHARTS_DEPENDENCIES
)
# TODO: include highstock.js for highstock and modules/*.js files for
# exporting and getting data from various source like csv files etc.
#
# Highstock.js includes the highcharts.js, so only one of them required.
# see: https://www.highcharts.com/errors/16
#
# Using Highmaps as a plugin for HighCharts so using map.js instead of
# highmaps.js
#
# , 'modules/exporting.js' : for the exporting button
# data.js for getting data as csv or html table.
# 'highcharts-more.js' : for arearange and some other chart type
Expand Down
8 changes: 8 additions & 0 deletions lib/daru/view/adapters/highcharts/layout_helper_iruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ def high_chart_iruby(chart_class, placeholder, object, &block)
).concat(content_tag('div', '', object.html_options))
end

def high_map(placeholder, object, &block)
object.html_options[:id] = placeholder
object.options[:chart][:renderTo] = placeholder
build_html_output(
'Map', placeholder, object, &block
).concat(content_tag('div', '', object.html_options))
end

private

def build_html_output_iruby(type, placeholder, object, &block)
Expand Down
Loading