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.

Sign up for GitHub

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

Jump to bottom

Added custom CSS in highcharts #93

Merged
merged 11 commits into from
Jul 6, 2018
Merged

Added custom CSS in highcharts #93

merged 11 commits into from
Jul 6, 2018

Conversation

Prakriti-nith
Copy link
Contributor

There are two modes of Highcharts: Classic and Styled.
Classic mode provides some custom CSS styling and does not require any additional dependencies while styled mode uses js dependency (like https://code.highcharts.com/stock/js/highstock.js) replacing the older one + the CSS dependency and provides full custom CSS styling.
I have implemented the Styled mode for web frameworks but as there is no method to load CSS files in IRuby notebook, so I have kept classic mode for IRuby notebook where some styling can be provided.
User needs to provide the required CSS styling in CSS option.
I will add the examples of IRuby notebook and rails application.

@Prakriti-nith
Copy link
Contributor Author

Examples Link : HighCharts - Custom Styling

@Shekharrajak
Copy link
Member

If you are adding more examples then use daru dataframe , vector as data source. Use some daru features to manipulate the data and various charts for quick overview of the data.

Shekharrajak
Shekharrajak reviewed May 26, 2018
View reviewed 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.

Run all the highcharts related examples in iruby and commit, to be sure nothing is breaking.

lib/daru/view/adapters/highcharts/display.rb Outdated
@@ -14,6 +16,9 @@ def self.init_script(
# Note: Don't reorder the dependent_js elements. It must be loaded in
# the same sequence. Otherwise some of the JS overlap and doesn't work.
js = ''
js << "\n<style type='text/css'>"
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 not js. Define separate method for css.

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 have created another method generate_init_code_css for loading css code. I have changed the variable name accordingly.

lib/daru/view/adapters/highcharts/iruby_notebook.rb Outdated
'highcharts-3d.js', 'modules/data.js']
dependent_js=['highstock.js', 'map.js', 'highcharts-more.js',
'modules/exporting.js', 'highcharts-3d.js', 'modules/data.js',
'maps/custom/europe.js']
Copy link
Member

Choose a reason for hiding this comment

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

This PR must be isolated from the highmap.

spec/adapters/highcharts_spec.rb
@@ -397,8 +397,8 @@
html = File.read(path)
expect(html).to match(/html/)
expect(html).to match(/Chart/)
expect(html).to match(/BEGIN highstock.js/i)
expect(html).to match(/END highstock.js/i)
expect(html).to match(/BEGIN js\/highstock.js/i)
Copy link
Member

Choose a reason for hiding this comment

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

Let us know why it is under js folder now.

Copy link
Contributor Author

@Prakriti-nith Prakriti-nith May 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There are two dependencies:

  1. http://code.highcharts.com/stock/highstock.js
  2. http://code.highcharts.com/stock/js/highstock.js

Former is used in classic mode while the latter one+CSS dependency is used in styled mode which I have implemented in web frameworks (so, I have put this dependency separately in js folder).

@Prakriti-nith
Copy link
Contributor Author

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

Shekharrajak
Shekharrajak reviewed Jun 27, 2018
View reviewed 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.

What is the coverage report after these changes ? It looks good to me, just consider the comments.

lib/daru/view.rb Outdated
@@ -112,7 +112,10 @@ def dependent_script(lib=:nyaplot)
when :nyaplot
Nyaplot.init_script
when :highcharts
LazyHighCharts.init_script
init_code = ''
Copy link
Member

Choose a reason for hiding this comment

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

We must have one class method, which will return init_css and init_script.
So we can use the same class method here as well.
DRY and Loose Coupling concepts should be applied.

lib/daru/view/adapters/highcharts/display.rb Outdated
if css_data != ''
script << '<style>'
# Applying the css to chart div
css_data.each do |css|
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 alignment and script must be css_script or specific name for css.

lib/tasks/high_charts.rake
end
end

task :css do
say "Grabbing CSS of HighCharts from Upstream..." 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 test it manually once again properly and confirm.

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 have tested the rake task and the examples again. Everything is working fine.

@Prakriti-nith
Copy link
Contributor Author

Right now, after this commit the coverage of daru-view is 96.15%

Shekharrajak
Shekharrajak reviewed Jul 1, 2018
View reviewed changes
lib/daru/view.rb Outdated
@@ -112,7 +112,7 @@ def dependent_script(lib=:nyaplot)
when :nyaplot
Nyaplot.init_script
when :highcharts
LazyHighCharts.init_script
LazyHighCharts.init_code
Copy link
Member

@Shekharrajak Shekharrajak Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think it must be init_script only (to have the same method name in all the places).

lib/daru/view/adapters/highcharts/display.rb Outdated
# Loads the dependent javascript required
#
# @param [Array] dependent js files required
# @return [String] js code of the dependent files
def self.init_script(
Copy link
Member

Choose a reason for hiding this comment

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

You can change this name to init_javascript.

@Prakriti-nith
Copy link
Contributor Author

access_row_tuples_by_indexs has been recently removed from the daru, so it is creating problems in the visualization of googlecharts and highcharts.

@Shekharrajak Shekharrajak mentioned this pull request Jul 2, 2018
Closed
Shekharrajak
Shekharrajak approved these changes Jul 6, 2018
View reviewed changes
Copy link
Member

@Shekharrajak Shekharrajak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I believe you have tested the IRuby notebook examples with these changes.

@Shekharrajak Shekharrajak merged commit c1f6a44 into SciRuby:master Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shekharrajak Shekharrajak Shekharrajak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Prakriti-nith @Shekharrajak