Remove gmaps4rails gem [fixes #134262481] #397

Maroo-b · 2016-11-19T18:34:38Z

Addresses: https://www.pivotaltracker.com/story/show/134262481

houndci-bot · 2016-11-19T18:34:44Z

app/assets/stylesheets/gmaps4rails.css.scss

+#map_canvas {
+  height: 400px;
+  width: 100%;
+}


Rule declaration should be followed by an empty line

houndci-bot · 2016-11-19T18:34:44Z

app/assets/stylesheets/gmaps4rails.css.scss

@@ -1,5 +1,9 @@
 @import "utils/colors";

+#map_canvas {


Avoid using id selectors
Selector map_canvas should be written in lowercase with hyphens

houndci-bot · 2016-11-19T18:34:44Z

app/assets/javascripts/maps.js

@@ -0,0 +1,17 @@
+'use strict';
+var map;
+function initMap() {


'initMap' is defined but never used.

houndci-bot · 2016-11-19T18:34:45Z

app/assets/javascripts/maps.js

+    zoom: 12
+  });
+  var markerData = $("#marker_data").data().markers;
+  console.log(markerData);


'console' is not defined.

houndci-bot · 2016-11-19T18:34:45Z

app/assets/javascripts/maps.js

+  var marker, i;
+  for (i = 0; i < markerData.length; i++) {  
+    marker = new google.maps.Marker({
+      position: new google.maps.LatLng(markerData[i].lat, markerData[i].lng),


'google' is not defined.

houndci-bot · 2016-11-19T18:34:45Z

app/assets/javascripts/maps.js

+  console.log(markerData);
+  var marker, i;
+  for (i = 0; i < markerData.length; i++) {  
+    marker = new google.maps.Marker({


'google' is not defined.

houndci-bot · 2016-11-19T18:34:45Z

app/assets/javascripts/maps.js

+'use strict';
+var map;
+function initMap() {
+  map = new google.maps.Map(document.getElementById('map_canvas'), {


'google' is not defined.

houndci-bot · 2016-11-19T18:34:46Z

app/assets/javascripts/maps.js

+    center: {lat: 51.5978, lng: -0.3370},
+    zoom: 12
+  });
+  var markerData = $("#marker_data").data().markers;


Mixed double and single quotes.

houndci-bot · 2016-11-19T18:34:46Z

app/assets/javascripts/maps.js

@@ -0,0 +1,17 @@
+'use strict';


Use the function form of "use strict".

houndci-bot · 2016-11-20T19:32:09Z

app/assets/stylesheets/gmaps4rails.css.scss

+  position: absolute;
+  bottom:0;
+  left:50%;
+  transform:translateX(-50%);


Colon after property should be followed by one space

houndci-bot · 2016-11-20T19:32:09Z

app/assets/stylesheets/gmaps4rails.css.scss

+  display: block;
+  position: absolute;
+  bottom:0;
+  left:50%;


Colon after property should be followed by one space

houndci-bot · 2016-11-20T19:32:09Z

app/assets/stylesheets/gmaps4rails.css.scss

@@ -30,6 +34,11 @@
 .measle {
  height: 6px;
  width: 6px;
+  display: block;
+  position: absolute;
+  bottom:0;


Colon after property should be followed by one space

houndci-bot · 2016-11-20T19:32:09Z

app/assets/javascripts/maps.js

@@ -0,0 +1,73 @@
+var map;
+function initMap() {


'initMap' is defined but never used.

houndci-bot · 2016-11-20T19:32:09Z

app/assets/javascripts/maps.js

+  var markerData = $("#marker_data").data().markers;
+  var marker, i;
+  for (i = 0; i < markerData.length; i++) {  
+    var latLng = new google.maps.LatLng(markerData[i].lat, markerData[i].lng);


'google' is not defined.

houndci-bot · 2016-11-20T19:32:10Z

app/assets/javascripts/maps.js

+  CustomMarker.prototype.getPosition = function() {
+    return this.latlng;	
+  };
+  map = new google.maps.Map(document.getElementById('map_canvas'), {


'google' is not defined.

houndci-bot · 2016-11-20T19:32:10Z

app/assets/javascripts/maps.js

+    this.setMap(map);	
+  }
+
+  CustomMarker.prototype = new google.maps.OverlayView();


'google' is not defined.

houndci-bot · 2016-11-20T19:32:10Z

app/assets/javascripts/maps.js

+      latLng,
+      map,
+      {
+        content: markerData[i].custom_marker


Identifier 'custom_marker' is not in camel case.

houndci-bot · 2016-11-20T19:32:10Z

app/assets/javascripts/maps.js

+
+
+      google.maps.event.addDomListener(div, "click", function(event) {			
+        google.maps.event.trigger(self, "click");


Mixed double and single quotes.
'google' is not defined.

houndci-bot · 2016-11-20T19:32:10Z

app/assets/javascripts/maps.js

+      div.innerHTML = self.args.content;
+
+
+      google.maps.event.addDomListener(div, "click", function(event) {			


Mixed double and single quotes.
'event' is defined but never used.
'google' is not defined.

houndci-bot · 2016-11-20T20:04:02Z

app/assets/stylesheets/gmaps4rails.css.scss

+  position: absolute;
+  bottom:0;
+  left:50%;
+  transform:translateX(-50%);


Colon after property should be followed by one space

houndci-bot · 2016-11-20T20:04:02Z

app/assets/stylesheets/gmaps4rails.css.scss

 .measle {
  height: 6px;
  width: 6px;
+  position: absolute;
+  bottom:0;
+  left:50%;


Colon after property should be followed by one space

houndci-bot · 2016-11-20T20:04:02Z

app/assets/stylesheets/gmaps4rails.css.scss

+  position: absolute;
+  bottom:0;
+  left:50%;
+  transform:translateX(-50%);


Colon after property should be followed by one space

houndci-bot · 2016-11-20T20:04:02Z

app/assets/stylesheets/gmaps4rails.css.scss

 .measle {
  height: 6px;
  width: 6px;
+  position: absolute;
+  bottom:0;


Colon after property should be followed by one space

houndci-bot · 2016-11-20T20:04:02Z

app/assets/stylesheets/gmaps4rails.css.scss

 .measle {
  height: 6px;
  width: 6px;
+  position: absolute;
+  bottom:0;
+  left:50%;


Colon after property should be followed by one space

houndci-bot · 2016-11-20T20:04:02Z

app/assets/stylesheets/gmaps4rails.css.scss

+.custom-marker {
+  height: 22px;
+  width: 22px;
+}


Rule declaration should be followed by an empty line

houndci-bot · 2016-11-20T20:04:02Z

app/assets/stylesheets/gmaps4rails.css.scss

 .measle {
  height: 6px;
  width: 6px;
+  position: absolute;
+  bottom:0;


Colon after property should be followed by one space

houndci-bot · 2016-11-20T20:04:03Z

app/assets/javascripts/maps.js

+    var latLng = new google.maps.LatLng(markerData[i].lat, markerData[i].lng);
+
+    marker = new google.maps.Marker({
+            position: new google.maps.LatLng(markerData[i].lat, markerData[i].lng),


'google' is not defined.

houndci-bot · 2016-11-20T20:04:03Z

app/assets/stylesheets/gmaps4rails.css.scss

+.custom-marker {
+  height: 22px;
+  width: 22px;
+}


Rule declaration should be followed by an empty line

johnnymo87 · 2016-11-29T10:44:36Z

app/assets/javascripts/google_maps/custom_marker.js

+  // check the explancation from: http://humaan.com/custom-html-markers-google-maps/
+  if (point) {
+    div.style.left = (point.x - ($('.custom-marker').width() / 2)) + 'px';
+    div.style.top = (point.y - $('.custom-marker').height()) + 'px';


Do these markers have a consistent height and width? If so, I wonder if we could avoid doing this calculation for each marker?

johnnymo87 · 2016-11-29T10:46:26Z

app/assets/javascripts/maps.js

+  boxText.setAttribute("class", "arrow_box")
+  ibOptions.content = boxText;
+
+  for (i = 0; i < markerData.length; i++) {  


How do you feel about using forEach here?

thx for the note I'll make an update.

johnnymo87 · 2016-11-29T13:11:17Z

@Maroo-b Testing this javascript may be difficult due to it's dependence on global variables, e.g. google, document, this.setMap, etc. I see some options:

Refactor to make all dependencies explicitly injected and mocked.
Load the real google map libraries in the test suite.
Load a mock of the google map libraries in the test suite.

I've briefly explored these options, here's what I've seen so far:

I refactored things and then and in the test I initialized things with a mock google object and a real document. This works until InfoBox. That library depends on google and document being globally available. Maybe anInfoBox instance should be mocked and injected as well?
Add - "//maps.google.com/maps/api/js?v=3&libraries=geometry<%= ENV['GMAP_API_KEY'].nil? ? '' : "&key=#{ENV['GMAP_API_KEY']}" %>" to src_files in spec/javascripts/support/jasmine.yml. I think the tests will need to wait for everything to load.
Define google = { ... }; in app/assets/javascripts/fake_google.js and load this in spec/javascripts/support/jasmine.yml. This works until this.setMap. This function and others seem to magically arrive with the real google library. I'm not familiar enough with javascript to understand how they magically get mixed into the CustomMarker class.

I feel like option 1 is our safest bet. I would separate the initialization from the implementation, so that the tests can load just the implementation, and initialize it with mocks. We can test the real initialization via selenium (cucumber).

Maroo-b · 2016-11-29T13:43:13Z

@johnnymo87 thanks for your feedback, I think the easiest solution is to extract marker_reader to a separate function but is it really worth it to be tested ? It seems it's an implementation test not behavior one no ?

johnnymo87 · 2016-11-29T13:53:35Z

Yeah, I agree it's an implementation test, but if I am not sure if there is any other way to assert that markers are drawn on the provided coordinates.

Maroo-b · 2016-11-29T14:18:50Z

From the test file (source), it looks like we are testing if function marker_data is able to read data from 'data-markers' attributes.

Regarding to check if markers are drawn I checked that the functionality is verified by those cucumber tests ( maps, volunteer ops)

for the moment I'll delete that file to make the test suite pass.

johnnymo87 · 2016-11-29T14:27:12Z

Yeah, we probably have a good deal of coverage via selenium. I'm not sure if it's 100%, though. For instance, what if we swapped the latitude and longitude of the coordinates when passing them to the google libraries ... would any cukes fail? If not, we probably need lower-level javascript tests.

Addresses: https://www.pivotaltracker.com/story/show/134262481

Maroo-b · 2016-12-03T07:38:36Z

@johnnymo87 good point,

When I was working on this PR I used the default google markers to check if the custom markers are correctly positioned, maybe we can think how to implement this test with jasmine.

Regarding swapping lat and lng inputs to google library, technically it will draws the marker to another position that why I suggested to compare default markers with custom ones.

tansaku · 2016-12-07T16:22:53Z

@Maroo-b many thanks for working on this - great to see that old coffee script thrown out - I'm much more comfortable with pure JS.

I notice that we now have a custom markers_builder.rb class - is that not what the gem was providing for us before? If memory serves we are removing the gem to make tighter list/map integration possible is that right?

We're making some custom ruby code here to give us more flexibility about how we use the map, is that right?

Maroo-b · 2016-12-07T20:31:54Z

@tansaku thank you for checking the PR.

I extracted the markers_builder.rb from gmaps4rails gem with it's spec file it's used to create the options for the markers.

The main change as you noted is in the javascript part, know we are using the native google js library so it's more easier to customize and check google maps docs for reference. For example I added the option to center a maker on click source which is now so easy and straightforward.

tansaku · 2016-12-08T11:19:55Z

@Maroo-b great! so we are good to go?

What do you think of these two code climate issues? do you think we can safely ignore them? @johnnymo87 ?

Could we ultimately be extracting our js code into an npm module that we could share with others?

Is the gmap4rails gem not being maintained? Should we be doing PRs against that?

tansaku · 2016-12-08T11:26:22Z

app/services/gmaps/markers_builder.rb

+        @hash   = {}
+      end
+
+      def call(&block)


maybe prefer run here? http://blog.silvabox.com/musings-on-service-objects/

@tansaku this file was extracted from Gmaps4rails gem, the only method I added was generate in class MarkersBuilder source
The MarkerBuilder instance accepts a block that's why it seems that naming the method to call here was chosen to reveal the intention of calling blocks.

tansaku · 2016-12-08T11:28:55Z

also, do we need some jasmine tests on custom_marker.js and maps.js ?

tansaku · 2016-12-08T11:35:44Z

also, can we find some way to get test coverage information for jasmine?

Maroo-b · 2016-12-09T20:37:52Z

@tansaku for a custom google maps integration, it seems easier to use the native google maps js library. And now with pure js it's simple to check customization of event handling and managing opening infobox based on list items.

So for jasmine tests, I find that maps integration is well covered with integration tests but it will be amazing if we could add tests for custom_marker.js because when I was working on this PR I used the default google markers to check if the custom markers are correctly positioned.

johnnymo87 · 2016-12-09T23:47:49Z

app/assets/javascripts/maps.js

+
+      }
+
+    })(marker, item)); 


Code climate noticed that this function is invoked when passed to the addListener function. It's interesting that this invoked function returns a function. @Maroo-b, does marker and item still work in the inner-most function without being enclosed in this invoked function?

@johnnymo87 I had to use this keyword to refer the object to which the event handler is attached. It seems this solution fixed the warning from code climate.

johnnymo87 · 2016-12-10T00:16:36Z

@Maroo-b Would you mind tagging all the relevant cucumber tests with an @maps tag?

I agree, the native google maps library is too easy to integrate with to merit making this a public library.

I think that javascript could use some testing. It will be challenging, though. The downside of the easy integration is that it's too easy. google is a global variable, and CustomMarker inherits from google.maps.OverlayView, introducing a lot of magic functionality. You can't test it without either (1) loading the real google maps libraries in the test suite, or (2) writing an extremely thin wrapper around it, depend on that instead, and test against that.

That said, this otherwise looks like a very nice implementation, so if the cucumber tests are sufficient, I say we ship it and open a separate ticket for the javascript testing.

@tansaku I believe the gmaps4rails gem is still maintained, but all we're doing here is taking one file from it and tossing the rest, which probably wouldn't be welcomed upstream.

Maroo-b · 2016-12-11T19:34:04Z

@johnnymo87 I update the PR to add maps tags to cucumber tests.

tansaku · 2016-12-13T15:12:01Z

@Maroo-b @johnnymo87 that's great - thanks for the explanation, I'll make a ticket for the javascript unit testing ... https://www.pivotaltracker.com/story/show/136036569

tansaku · 2016-12-13T15:12:27Z

I'm just going to run this locally to check for anything we might have missed and then get it merged in

houndci-bot reviewed Nov 19, 2016

View reviewed changes

app/assets/javascripts/maps.js

@@ -0,0 +1,17 @@

'use strict';

var map;

function initMap() {

Copy link

houndci-bot Nov 19, 2016

Choose a reason for hiding this comment

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

'initMap' is defined but never used.

houndci-bot reviewed Nov 19, 2016

View reviewed changes

app/assets/javascripts/maps.js

@@ -0,0 +1,17 @@

'use strict';

Copy link

houndci-bot Nov 19, 2016

Choose a reason for hiding this comment

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

Use the function form of "use strict".

Maroo-b added the WIP label Nov 19, 2016

houndci-bot reviewed Nov 20, 2016

View reviewed changes

app/assets/javascripts/maps.js

@@ -0,0 +1,73 @@

var map;

function initMap() {

Copy link

houndci-bot Nov 20, 2016

Choose a reason for hiding this comment

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

'initMap' is defined but never used.

houndci-bot reviewed Nov 20, 2016

View reviewed changes

app/assets/javascripts/maps.js

this.setMap(map);

}

CustomMarker.prototype = new google.maps.OverlayView();

Copy link

houndci-bot Nov 20, 2016

Choose a reason for hiding this comment

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

'google' is not defined.

houndci-bot reviewed Nov 20, 2016

View reviewed changes

app/assets/javascripts/maps.js

latLng,

map,

{

content: markerData[i].custom_marker

Copy link

houndci-bot Nov 20, 2016

Choose a reason for hiding this comment

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

Identifier 'custom_marker' is not in camel case.

houndci-bot reviewed Nov 20, 2016

View reviewed changes

Maroo-b force-pushed the 134262481_remove_gmaps4rails_gem branch from 391fe49 to 6df044f Compare November 20, 2016 20:03

houndci-bot reviewed Nov 20, 2016

View reviewed changes

Maroo-b added RFC and removed WIP labels Nov 28, 2016

johnnymo87 changed the title ~~WIP remove gmaps4rails gem [fixes #134262481]~~ Remove gmaps4rails gem [fixes #134262481] Nov 29, 2016

johnnymo87 reviewed Nov 29, 2016

View reviewed changes

Maroo-b force-pushed the 134262481_remove_gmaps4rails_gem branch from a31c237 to 5a9fd55 Compare December 3, 2016 06:49

remove gmaps4rails gem [fixes #134262481]

118ac7a

Addresses: https://www.pivotaltracker.com/story/show/134262481

Maroo-b force-pushed the 134262481_remove_gmaps4rails_gem branch from 5a9fd55 to 118ac7a Compare December 3, 2016 07:08

tansaku temporarily deployed to harrowcn-develop-pr-397 December 7, 2016 16:23 Inactive

tansaku reviewed Dec 8, 2016

View reviewed changes

johnnymo87 reviewed Dec 9, 2016

View reviewed changes

Add @maps tags and fix code climate warning

44d1cd9

tansaku merged commit 9d534c5 into AgileVentures:develop Dec 16, 2016

bryszard mentioned this pull request Jan 8, 2017

68370622 upgrade to bootstrap 3.x #408

Merged



		google.maps.event.addDomListener(div, "click", function(event) {
		google.maps.event.trigger(self, "click");

		div.innerHTML = self.args.content;


		google.maps.event.addDomListener(div, "click", function(event) {

Remove gmaps4rails gem [fixes #134262481] #397

Remove gmaps4rails gem [fixes #134262481] #397

Conversation

Maroo-b commented Nov 19, 2016 • edited Loading

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

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

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

johnnymo87 commented Nov 29, 2016 • edited Loading

Maroo-b commented Nov 29, 2016

johnnymo87 commented Nov 29, 2016

Maroo-b commented Nov 29, 2016

johnnymo87 commented Nov 29, 2016 • edited Loading

Maroo-b commented Dec 3, 2016

tansaku commented Dec 7, 2016

Maroo-b commented Dec 7, 2016

tansaku commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tansaku commented Dec 8, 2016

tansaku commented Dec 8, 2016

Maroo-b commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnymo87 commented Dec 10, 2016 • edited Loading

Maroo-b commented Dec 11, 2016

tansaku commented Dec 13, 2016

tansaku commented Dec 13, 2016

Maroo-b commented Nov 19, 2016 •

edited

Loading

johnnymo87 commented Nov 29, 2016 •

edited

Loading

johnnymo87 commented Nov 29, 2016 •

edited

Loading

johnnymo87 commented Dec 10, 2016 •

edited

Loading