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 Highstock feature #89

Merged
merged 11 commits into from May 22, 2018

Conversation

Projects
None yet
2 participants
@Prakriti-nith
Contributor

Prakriti-nith commented May 14, 2018

Added highstock feature.
The value of chart_class will be provided by the user.
Here are the links to view examples: Highstock-Chart types, Highstock-General

Prakriti-nith added some commits May 14, 2018

@Prakriti-nith Prakriti-nith force-pushed the Prakriti-nith:highstock branch from d97390c to c7fc3cd May 14, 2018

@Prakriti-nith

This comment has been minimized.

Contributor

Prakriti-nith commented May 14, 2018

I have added some more examples here.

@Shekharrajak

Please add all the examples in iruby . Also run the previously present examples (to check nothing is breaking) and commit the changes.

Good work!

@@ -48,7 +69,15 @@ def show_in_iruby(placeholder=random_canvas_id)
def to_html_iruby(placeholder=random_canvas_id)
# TODO : placeholder pass, in plot#div
chart_hash_must_be_present
high_chart_iruby(placeholder, self)
chart_class = options.delete(:chart_class).to_s.downcase unless

This comment has been minimized.

@Shekharrajak

Shekharrajak May 15, 2018

Collaborator

Make it DRY. May be one method or instance variable.

end
def high_graph_iruby(placeholder, object, &block)
build_html_output_iruby('Chart', placeholder, object, &block)
end
def high_graph_stock_iruby(placeholder, object, &block)

This comment has been minimized.

@Shekharrajak

Shekharrajak May 15, 2018

Collaborator

high_graph_iruby also doing the same thing . We can add chart_class parameter, right?

@@ -29,18 +43,18 @@ def encapsulate_js_iruby(core_js)
"#{js_start_iruby} #{core_js} #{js_end_iruby}"
# Turbolinks.version < 5
elsif defined?(Turbolinks) && request_is_referrer?
to_s(eventlistener_page_load)
eventlistener_page_load(core_js)

This comment has been minimized.

@Shekharrajak

Shekharrajak May 15, 2018

Collaborator

without passing core_js , it will work, right ?

This comment has been minimized.

@Prakriti-nith

Prakriti-nith May 15, 2018

Contributor

No, it was not working as these methods (eventlistener_page_load, eventlistener_turbolinks_load, call_core_js) required core_js.

@placeholder = "placeholder"
end
describe "#to_html" do

This comment has been minimized.

@Shekharrajak

Shekharrajak May 15, 2018

Collaborator

Please write specs for all methods present in display.rb , iruby_notebook.rb, layout_helper_iruby.rb (or few specs such that it covers all the methods).

before { Daru::View.plotting_library = :highcharts }
before(:each) do
@placeholder = "placeholder"

This comment has been minimized.

@Shekharrajak

Shekharrajak May 15, 2018

Collaborator

Please use sample data, options which can generate the charts without error (don't just test for generated js).

@Prakriti-nith

This comment has been minimized.

Contributor

Prakriti-nith commented May 15, 2018

I have added almost all the examples of HighStock except those which require additional javascript (apart from options and data (series)) like this. I also tried all the examples but one example from nyaplot (in box plot example) was throwing some error. I am not sure if the error is related to this feature addition.

@Shekharrajak

I am expecting spec for plot.to_html which will generate js code having all the set options like chart, rangeSelector, title, data, dataGrouping, etc.

end
def extract_chart_class
options.delete(:chart_class).to_s.downcase unless options[:chart_class].nil?

This comment has been minimized.

@Shekharrajak

Shekharrajak May 16, 2018

Collaborator

chart_class = options.delete(:chart_class).to_s.capitalize unless options[:chart_class].nil?
chart_class = 'StockChart' if chart_class == 'stock'

def high_graph_stock_iruby(placeholder, object, &block)
build_html_output_iruby('StockChart', placeholder, object, &block)
if chart_class == 'map'

This comment has been minimized.

@Shekharrajak

Shekharrajak May 16, 2018

Collaborator

build_html_output_iruby(chart_class , placeholder, object, &block).concat(content_tag('div', '', object.html_options))

before { Daru::View.plotting_library = :highcharts }
describe "#init_script" do
it "generates valid initial script" do
js = LazyHighCharts.init_script

This comment has been minimized.

@Shekharrajak

Shekharrajak May 16, 2018

Collaborator

This is not testing whether all the js files are loaded or not.

Isn't we check for "BEGIN highstock.js" and "END highstock.js" , ... ?

@placeholder,
@chart.chart)
).to match(/window.chart_placeholder\s=/)
end
it "should set Chart data" do
expect(@chart.chart.high_chart_iruby(
"",

This comment has been minimized.

@Shekharrajak

Shekharrajak May 16, 2018

Collaborator

must be "Chart"

@Shekharrajak

This comment has been minimized.

Collaborator

Shekharrajak commented May 16, 2018

I also tried all the examples but one example from nyaplot (in box plot example) was throwing some error. I am not sure if the error is related to this feature addition.

Commit the changes (when you run IRuby notebook examples , .ipynb files will get changed)

@Shekharrajak

This comment has been minimized.

Collaborator

Shekharrajak commented May 16, 2018

It seems you haven't done bundle install in spec/dummy_iruby .

@Prakriti-nith Prakriti-nith force-pushed the Prakriti-nith:highstock branch from cd78b93 to 6516318 May 16, 2018

@Prakriti-nith

This comment has been minimized.

Contributor

Prakriti-nith commented May 18, 2018

@Shekharrajak , please review it and let me know any changes I have to make.

@Shekharrajak

Consider minor comments, then it will be merged.

elsif chart_class == 'StockChart'
high_stock(placeholder, self)
# No need to pass any value for HighChart
else

This comment has been minimized.

@Shekharrajak

Shekharrajak May 20, 2018

Collaborator

I think it must be specific 'Chart'. If user have entered some xyz chart_class then raise error 'Not implemented' or '3 chart_class chart, stock, map must be selected ' something like that.

{
name: 'AAPL Stock Price',
data: [
[1147651200000,67.79],

This comment has been minimized.

@Shekharrajak

Shekharrajak May 20, 2018

Collaborator

Proper indentation please.

elsif chart_class == 'Chart'
high_chart(placeholder, self)
else
raise 'chart_class must be selected as either chart, stock or map'

This comment has been minimized.

@Shekharrajak

Shekharrajak May 20, 2018

Collaborator

Isn't this raise must be in extract_chart_class method? Remember one method must do only one thing.

# @return [String] the class of the chart
def extract_chart_class
# Provided by user and can take two values ('stock' or 'map').
chart_class = options.delete(:chart_class).to_s.downcase unless

This comment has been minimized.

@Shekharrajak

Shekharrajak May 20, 2018

Collaborator

Instead of downcase , capitalize can reduce the number of line. Please read my comments.

This comment has been minimized.

@Prakriti-nith

Prakriti-nith May 20, 2018

Contributor

Yes, but we need to check whether chart_class is assigned or not and user has entered the correct value for chart_class. For that, we still have to compare with all the 3 classes.

Prakriti-nith added some commits May 20, 2018

end
chart_class = 'StockChart' if chart_class == 'Stock'
chart_class = 'Chart' if chart_class.nil?
unless %w[Chart StockChart Map].include?(chart_class) || chart_class.nil?

This comment has been minimized.

@Shekharrajak

Shekharrajak May 22, 2018

Collaborator

|| chart_class.nil? is not required.

chart_class = 'StockChart' if chart_class == 'Stock'
chart_class = 'Chart' if chart_class.nil?
unless %w[Chart StockChart Map].include?(chart_class) || chart_class.nil?
raise 'chart_class must be selected as either chart, stock or map'

This comment has been minimized.

@Shekharrajak

Shekharrajak May 22, 2018

Collaborator

Testcase for this?

@Shekharrajak Shekharrajak merged commit 20acde5 into SciRuby:master May 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Shekharrajak

This comment has been minimized.

Collaborator

Shekharrajak commented May 22, 2018

Thanks @Prakriti-nith ! Merging it.

@Shekharrajak

This comment has been minimized.

Collaborator

Shekharrajak commented Jul 14, 2018

Also fixes #8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment