-
Notifications
You must be signed in to change notification settings - Fork 19
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
Added HighMaps #92
Conversation
Revised Examples (containing |
When the plotting_library (or dependent_script) is set to highcharts, the dependencies (highstock.js, map.js) and the dependencies of the basic modules that are common to all the three (HighCharts, HighStock, HighMaps) will be loaded. Then the user can provide the required modules of the chart in options. These will be loaded in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write examples easy to advance. Also use daru dataframe, vector in most of the places along with list or json object.
@@ -3,7 +3,7 @@ | |||
|
|||
module LazyHighCharts | |||
def self.init_script( | |||
dependent_js=['highstock.js', 'highcharts-more.js', 'modules/exporting.js', | |||
dependent_js=['highstock.js', 'map.js', 'modules/exporting.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is constant we must place all the constant variable into one file, to make it loosely coupled. Accessing these constants into multiple files will be better.
@@ -61,6 +64,17 @@ def to_html_iruby(placeholder=random_canvas_id) | |||
high_chart_iruby(extract_chart_class, placeholder, self) | |||
end | |||
|
|||
# @return load the dependent modules of the chart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least Input parameter and return object must be documented according to yard.
In this commit I have:
Examples: Countries, General, Series, Dynamic. |
Please use directly |
I tried using |
dep_js = [] | ||
# Mapdata dependencies | ||
if !options[:chart_class].nil? && options[:chart_class].capitalize == 'Map' | ||
dep_js.push('mapdata/' + options[:chart][:map].to_s + '.js') if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user input countries/folder1/folder2
things is making it complicated. Think about other solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have kept the same folder structure provided in the highcharts dependencies. It is because the value provided by the user (it is in highcharts option and will be provided everytime user needs some mapdata dependency) in options[:chart][:map] will be the same as of the dependencies and by this folder structure it will be easy to retrieve the data.
end | ||
# Dependencies provided in modules option (of highcharts mainly | ||
# like tilemap) by the user | ||
dep_js |= options.delete(:modules).map! { |js| "#{js}.js" } unless |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 | ||
# rubocop:enable Metrics/AbcSize |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/tasks/high_charts.rake
Outdated
sh "mkdir -p lib/daru/view/adapters/js/highcharts_js/mapdata/countries/" | ||
sh "mkdir -p lib/daru/view/adapters/js/highcharts_js/mapdata/custom/" | ||
|
||
countries_folders = ['in', 'us', 'gb', 'af', 'au', 'bd', 'be', 'ca', 'cn', 'fr'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only these many countries ? What will be total size of js if we download all the js ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think around 10 - 15 MBs. Ok, I will download them all.
|
||
module LazyHighCharts | ||
def self.init_script( | ||
dependent_js=['highstock.js', 'highcharts-more.js', 'modules/exporting.js', | ||
'highcharts-3d.js', 'modules/data.js'] | ||
dependent_js=[HIGHSTOCK, MAP, EXPORTING, HIGHCHARTS_3D, DATA] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[HIGHSTOCK, MAP, EXPORTING, HIGHCHARTS_3D, DATA]
is also constant as web app highcharts dependencies.
I have added the support for multiple series in highcharts. I have also updated all the examples of highcharts, highstock and highmaps using |
@Shekharrajak please review the changes. |
lib/daru/view/adapters/highcharts.rb
Outdated
series_name = options[:name] unless options[:name].nil? | ||
f.series(type: series_type, name: series_name, data: data_new) | ||
# For multiple series | ||
if data.is_a?(Array) && data[0].is_a?(Hash) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -33,22 +33,25 @@ class HighChart | |||
# | |||
def to_html(placeholder=random_canvas_id) | |||
chart_hash_must_be_present | |||
script = load_dependencies('web_frameworks') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
end | ||
# Dependencies provided in modules option (of highcharts mainly | ||
# like tilemap) by the user | ||
dep_js |= options.delete(:modules).map! { |js| "#{js}.js" } unless |
There was a problem hiding this comment.
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?
options[:modules].nil? | ||
dep_js | ||
end | ||
# rubocop:enable Metrics/AbcSize |
There was a problem hiding this comment.
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.
@@ -35,6 +35,7 @@ | |||
chart: { | |||
type: 'bar' | |||
}, | |||
modules: ['modules/treemap'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
treemap for bar chart?
lib/daru/view/adapters/highcharts.rb
Outdated
# 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) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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).
options[:chart] && | ||
options[:chart][:map] | ||
end | ||
# rubocop:enable Metrics/AbcSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this line.
There was a problem hiding this comment.
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 [void] Appends the map data in dep_js | ||
# rubocop:disable Metrics/AbcSize | ||
def get_map_data_dependencies(dep_js) | ||
dep_js.push('mapdata/' + options[:chart][:map].to_s + '.js') if |
There was a problem hiding this comment.
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.
I want to discuss about the future implementation (that we can do to improve it),if anything in TODO for map part. Update them here: https://github.com/SciRuby/daru-view/wiki/HighCharts-features |
I wanted to edit the wiki online but it says |
Wait, I didn't know about that. It will be public soon. |
@Shekharrajak I have made the required changes. Please review. |
Please add some solid examples that can show the full features of daru-view/highcharts/highmap in new wiki page ( Starting from easy to advance). It will be very useful , since there are lot of new things added in this PR. Once you resolve the conflicts, it will be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
|
||
# @param dep_js [Array] JS dependencies required for drawing a map(mapdata) | ||
# @return [void] Appends the map data in dep_js | ||
def get_map_data_dependencies(dep_js) |
There was a problem hiding this comment.
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.
@@ -57,4 +63,55 @@ namespace :highcharts do | |||
end | |||
end | |||
|
|||
task :map do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to find a way to handle many js files . It is increasing the gem size.
Noting down few important points, for future plan : Anything you want to add here , @Prakriti-nith ? |
Examples link: 1, 2, 3,
I am working on loading less js files.