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 Chartwrapper #95

Merged
merged 9 commits into from Jul 14, 2018

Conversation

Projects
None yet
3 participants
@Prakriti-nith
Contributor

Prakriti-nith commented Jun 5, 2018

As chart_class was already reserved in google_visualr, I used another option class_chart which will be provided in the third parameter. class_chart needs to be set as Chartwrapper do draw a Chartwrapper. Chartwrapper can be used to utilize additional option view. Examples are shown here. I am working on documenting the methods.

@Prakriti-nith Prakriti-nith force-pushed the Prakriti-nith:chartwrapper branch from ee534f2 to 3dd8fff Jun 7, 2018

@Prakriti-nith

This comment has been minimized.

Contributor

Prakriti-nith commented Jun 8, 2018

Explicitly loaded the 'table' package in chartwrapper also as it was not loading the 'table' package properly using autoload (Useful link).

@Prakriti-nith Prakriti-nith force-pushed the Prakriti-nith:chartwrapper branch from f13536c to 7680fd2 Jun 8, 2018

@Shekharrajak

This comment has been minimized.

Collaborator

Shekharrajak commented Jun 10, 2018

class_chart needs to be set as Chartwrapper do draw a Chartwrapper.

It will be confusing for user, isn't it?

@Prakriti-nith

This comment has been minimized.

Contributor

Prakriti-nith commented Jun 10, 2018

I have taken this as an analogy to HighCharts. We can use class_chart for both Chartwrapper and Charteditor with the default value as Chart.

@Prakriti-nith Prakriti-nith force-pushed the Prakriti-nith:chartwrapper branch from bba1e90 to f997906 Jun 13, 2018

Prakriti-nith added some commits Jun 5, 2018

Added Chartwrapper
Added documentation

Load packages explicitly in chartwrapper

Use chart_class instead of class_chart (for user)

@Prakriti-nith Prakriti-nith force-pushed the Prakriti-nith:chartwrapper branch from f997906 to 0ddb72a Jun 17, 2018

@Prakriti-nith

This comment has been minimized.

Contributor

Prakriti-nith commented Jun 27, 2018

@Shekharrajak can you please review this PR and let me know if there are any changes that I have to make?

@Shekharrajak

Please check once , whether you have added specs for all the new methods and new lines.

# @see #GoogleVisualr::DataTable.query_response_function_name
def query_response_function_name(element_id)
"handleQueryResponse_#{element_id.tr('-', '_')}"
end
# @param data [Array, Daru::DataFrame, Daru::Vector, Daru::View::Table, String]

This comment has been minimized.

@Shekharrajak

Shekharrajak Jul 1, 2018

Collaborator

All same methods (for chart and table) should be put into one module and include it into the classes where you want.

def draw_js_chart_wrapper(data, element_id)
js = ''
js << "\n function #{chart_function_name(element_id)}() {"
js << "\n #{to_js}"

This comment has been minimized.

@Shekharrajak

Shekharrajak Jul 1, 2018

Collaborator

tabs \t would be better instead of spaces.

@@ -35,14 +38,13 @@ def show_script(dom=SecureRandom.uuid, options={})
# Chart should be rendered in
# @return [String] js code to render the chart with script tag
def show_script_with_script_tag(dom=SecureRandom.uuid)
# if it is data table and importing data from spreadsheet
if is_a?(GoogleVisualr::DataTable) && data.is_a?(String)

This comment has been minimized.

@Shekharrajak

Shekharrajak Jul 1, 2018

Collaborator

How your changes is handling this if condition?

This comment has been minimized.

@Prakriti-nith

Prakriti-nith Jul 1, 2018

Contributor

I have kept the same method name for both table and chart, and now have put that method in the common module.

This comment has been minimized.

@Shekharrajak

Shekharrajak Jul 12, 2018

Collaborator

@Prakriti-nith , I don't think your replay is relevant . Now are you using the method to_js_full_script_spreadsheet ?

This comment has been minimized.

@Prakriti-nith

Prakriti-nith Jul 12, 2018

Contributor

I have removed to_js_full_script_spreadsheet method from data_table_iruby here and have added to_js_spreadsheet in the common module (display) here. Now, this method can be used both for tables as well as charts which is checked here.

Prakriti-nith added some commits Jul 1, 2018

@@ -0,0 +1,68 @@
require 'google_visualr'

This comment has been minimized.

@Shekharrajak

Shekharrajak Jul 6, 2018

Collaborator

There is already one module is added for display purpose , please check display module.
Isn't it display module contains similar methods?

This comment has been minimized.

@Prakriti-nith

Prakriti-nith Jul 7, 2018

Contributor

I have moved the common methods to display module. Sorry, I got confused. Actually, display module contained methods that called the methods of base_chart.rb and data_table.rb (like to_js and others) and there was no method that was called back from these files to display.

Prakriti-nith added some commits Jul 7, 2018

@Prakriti-nith

This comment has been minimized.

Contributor

Prakriti-nith commented Jul 11, 2018

Updated target ruby version to 2.2 in .rubocoop.yml file as travis 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.

@Prakriti-nith

This comment has been minimized.

Contributor

Prakriti-nith commented Jul 12, 2018

@Shekharrajak can you please review the changes?

@Shekharrajak

This comment has been minimized.

Collaborator

Shekharrajak commented Jul 13, 2018

Please resolve the conflicts and test it manually once.

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2018

Pull Request Test Coverage Report for Build 603

  • 324 of 325 (99.69%) changed or added relevant lines in 15 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 96.924%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/daru/view/adapters/googlecharts/display.rb 34 35 97.14%
Totals Coverage Status
Change from base Build 601: 0.4%
Covered Lines: 2332
Relevant Lines: 2406

💛 - Coveralls
@@ -12,7 +12,7 @@ module DatatablesAdapter
# the datatables option concept.
#
# TODO : this docs must be improved
def init_table(data=[], options={})
def init_table(data=[], options={}, _user_options={})

This comment has been minimized.

@Shekharrajak

Shekharrajak Jul 14, 2018

Collaborator

In _user_options , _ is not required, right ?

This comment has been minimized.

@Prakriti-nith

Prakriti-nith Jul 14, 2018

Contributor

Unless user_options is used in the method (in which it has been passed as a parameter), we need to prefix it with _ as rubocop was throwing Lint/UnusedMethodArgument error.

@Shekharrajak Shekharrajak merged commit 4b88756 into SciRuby:master Jul 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 96.924%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment