Refac thumbnail fv 73 #93

ede0m · 2017-07-31T18:19:08Z

Mostly involved adding reading in default_display_ids from examples into thumbnail.js

Can run and test the node script with the same command used in this pull request

… floodviz_onready

skaymen

Refactored code looks good besides the single comment I made.

PNG generation works correctly, although the old instructions for running it need to be updated.

skaymen · 2017-07-31T18:38:26Z

floodviz/thumbnail/thumbnail.js

@@ -82,7 +83,7 @@ function convert(figure, window, css_path, filename) {
 		svg_string = inject_style(style_default, null, svg, window);
 	}
 	// Takes care of canvas conversion and encodes base64
-	svg2png(svg_string)
+	svg2png(svg_string, {height: 300, width: 560})


The numbers for height and width are used twice in this file-- maybe we could make them vars?

There was refactoring done to height and width within the figure module while I was working on implementing the thumbnail script. When jumping on this current ticket, height and width had been removed from hydrograph.js during svg initialization (lines 116-122). This required me to specify height in width in svg2png because it could not be interpreted from the svg itself.

I did however, take your suggestion and declare vars at the top for height and width instead of using the actual integers.

mbucknell

Some minor things and some suggestions for a little reorganization of thumbnail.js

mbucknell · 2017-07-31T19:47:10Z

floodviz/static/js/floodviz_onready.js

 		'div_id': '#hydrograph'
 	};

 	var map_figure = FV.mapmodule(map_options);
-	var hydro_figure = FV.hydromodule(hydro_options);
+	var hydro_figure = hydromodule(hydro_options);

 	// Use frames to link interactions
 	var map_to_hydro = {


This is actually for https://github.com/ede0m/active-flood-viz/blob/33e572dbf2ce36d182ebeab1ea5311b601928a64/floodviz/static/js/floodviz_onready.js#L45. I would like to see the data passed directly into the init function. It's more explicit to pass it directly in and is not used before init() is called anyways.

I agree with this refactor. But I think it fits in better with a new ticket that would encapsulate the FV.default_display_id changes that still need to be made in map, for the sake of consistency across these figure modules. I would happily pick this new ticket up and make those changes tomorrow.

mbucknell · 2017-07-31T19:48:57Z

floodviz/static/js/hydrograph.js

+ *    Non-optional Keys include:
+ *        @prop 'height' v(int) - height of the graph
+ *        @prop 'width' v(int) - width of the graph
+ *        @prop 'data' v(list) - A list of objects representing data points


This is the one that I would like to see removed and instead passed in through the init() function. Please add a comment for the new display_ids property.

mbucknell · 2017-07-31T20:04:39Z

floodviz/thumbnail/thumbnail.js

@@ -15,6 +15,7 @@ var jsdom = require('jsdom/lib/old-api.js');
 var svg2png = require('svg2png');
 // Data imports
 var data_hydro = require('../thumbnail/hydrograph_data.json');
+var reference = require('../../examples/reference.json');


This location needs instance/reference.json as that is where we are putting it in the dockerfile process.

mbucknell · 2017-07-31T20:05:41Z

floodviz/thumbnail/thumbnail.js

+		if (args[0] === '-css') {
+			style_path = args[1];
+		} else {
+			console.log('\nUnrecognized argument: ' + args[0] + '\n');


Let's show the usage here too.

mbucknell · 2017-07-31T20:10:14Z

floodviz/thumbnail/thumbnail.js

@@ -44,19 +51,17 @@ jsdom.env(
 	[
 		'./floodviz/static/bower_components/d3/d3.js',
 		'./floodviz/static/bower_components/proj4/dist/proj4.js',


We don't need proj4 for the hydrograph, so this can be removed. Also remove the map div since we are only doing the hydrograph (that's above this on line 47).

mbucknell · 2017-07-31T20:20:53Z

floodviz/thumbnail/thumbnail.js

@@ -82,7 +87,7 @@ function convert(figure, window, css_path, filename) {
 		svg_string = inject_style(style_default, null, svg, window);


This comment is for line 78 where the additional css file is read in. Can this be done after line 30.? This gets the reading of the file done where the config parameter is read. You should be able to assign the value to style_ext and then you only need a single line to inject_style(style_default, style_ext, svg, window).

Good Idea! Thanks

mbucknell

Just need to add the one comment.

mbucknell · 2017-08-01T17:52:07Z

floodviz/static/js/hydrograph.js

+	 *                                        Pass null if there are no such interactions to link.
+	 *        @prop 'hover_in' - linked interaction function for hover_in events on this figure.
+	 *        @prop 'hover_out' - linked interaction function for hover_out events on this figure.
+	 *        @prop 'click' - linked interaction function for click events on this figure.


Add comment for @param data

ede0m added 4 commits July 21, 2017 11:45

Merge branch 'master' of https://github.com/USGS-VIZLAB/active-flood-viz

765e219

Merge branch 'master' of https://github.com/USGS-VIZLAB/active-flood-viz

dde0c0f

Merge branch 'master' of https://github.com/USGS-VIZLAB/active-flood-viz

539b0fd

read in references to thumbnail for display ids. Passed as options in…

a262180

… floodviz_onready

skaymen reviewed Jul 31, 2017

View reviewed changes

varible simplification

33e572d

mbucknell requested changes Jul 31, 2017

View reviewed changes

pass data to init. Removed some uncessary dependenceis. comments

ba2d50e

mbucknell requested changes Aug 1, 2017

View reviewed changes

comment

5ca24c3

mbucknell approved these changes Aug 1, 2017

View reviewed changes

mbucknell merged commit 7a70155 into USGS-VIZLAB:master Aug 1, 2017

ede0m deleted the refac_thumbnail_fv_73 branch August 1, 2017 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refac thumbnail fv 73 #93

Refac thumbnail fv 73 #93

ede0m commented Jul 31, 2017

skaymen left a comment

skaymen Jul 31, 2017

ede0m Jul 31, 2017

ede0m Jul 31, 2017

mbucknell left a comment

mbucknell Jul 31, 2017

ede0m Jul 31, 2017

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

ede0m Jul 31, 2017

mbucknell Jul 31, 2017

ede0m Jul 31, 2017

mbucknell Jul 31, 2017

mbucknell Jul 31, 2017

ede0m Aug 1, 2017

mbucknell left a comment

mbucknell Aug 1, 2017

		@@ -82,7 +87,7 @@ function convert(figure, window, css_path, filename) {
		svg_string = inject_style(style_default, null, svg, window);

Refac thumbnail fv 73 #93

Refac thumbnail fv 73 #93

Conversation

ede0m commented Jul 31, 2017

skaymen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbucknell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbucknell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment