I have a considerably large directive which is doing many things: First it renders a Google Map, then it adds a listener to check when the bounds of the map change, then it renders points on the map based where the bounds currently are, and finally it has a menu of options which allow the user to turn on and off various settings on the map.
The code snippet below has been heavily modified to show the most important aspects of the directive. I've also hardcoded the template rather than used a templateURL
.
This directive works exactly the way I want it to, and my questions are more about best practices for Angular:
- Should I be pulling out the Ajax calls and placing them inside a service?
- Should I create a Map controller and put some of this configuration logic in there?
- When do I know if I need a new controller for that matter?
I'm afraid there is too much going on inside this directive and it should be broken out.
(function(angular) {
'use strict';
angular.module('myApp.directives').directive('myAppMap', [function() {
return {
template: '<div>A whole bunch of toggles for the map go here</div> \
<div id="map-canvas" style="width: 100%; height: 600px"></div>'
link: function(scope, element, attrs) {
var map, markers = [], currentLayer = [];
function initialize() {
var mapOptions = {
zoom: 12,
center: new google.maps.LatLng(34.05, -118.245),
mapTypeId: google.maps.MapTypeId.ROADMAP,
mapTypeControlOptions: {
mapTypeIds: [google.maps.MapTypeId.ROADMAP, 'myApp_style']
}
},
map = new google.maps.Map(document.getElementById('map-canvas'),mapOptions)
map.mapTypes.set('myApp_style', styledMap);
map.setMapTypeId('myApp_style');
google.maps.event.addListener(map, 'bounds_changed', (function() {
_getClusters();
})());
}
function _getClusters(filters) {
$.ajax({
url: "http://localhost:1625/ocpu/library/imyApp/R/gen_cluster/json",
type: 'POST',
dataType:'json',
data: {
NE_lng:map.getBounds().getNorthEast(),
SW_lng:map.getBounds().getSouthWest()
maxNumber: 100
},
success:function(data){
_setMarkers(map, JSON.parse(data));
},
error: function (xhr, ajaxOptions, thrownError) {
console.log("ajax error - arun", xhr.status, thrownError);
}
})
}
function _setMarkers(map, clusters) {
for (var i = 0; i < clusters.length; i++) {
var thisCluster = clusters[i];
var myLatLng = new google.maps.LatLng(thisCluster.lat, thisCluster.lon);
markers[i] = new MarkerWithLabel({
position: myLatLng,
map: map,
icon: "marker_01.png",
labelClass: "myAppMarker",
labelAnchor: new google.maps.Point(10, 40),
})
google.maps.event.addListener(markers[i], 'click', function() {
map.setCenter(this.getPosition())
map.setZoom(map.getZoom()+1);
})
}
}
function _toggleControls(direction) {
if (direction=="open") {
$(".mapControls").height("130");
$(".mapControls .controls").fadeIn();
}
else {
$(".mapControls .controls").fadeOut();
$(".mapControls").height("30");
}
}
$(".mapControls .open").click(function(){
if ($(".mapControls .controls").is(":visible")) {
_toggleControls("close")
}
else {
_toggleControls("open")
}
})
initialize();
}
}
}]);
})(window.angular);
-
\$\begingroup\$ Your code does not run, your return statement seems wrong. \$\endgroup\$konijn– konijn2014年01月14日 14:50:14 +00:00Commented Jan 14, 2014 at 14:50
1 Answer 1
From a once over
- You seem to be missing a comma after
</div>'
- You seem to be missing a comma after
SW_lng:map.getBounds().getSouthWest()
- This code is reviewable, still, next time make sure there are no syntax errors..
- Hard coding
(34.05, -118.245)
is wrong, get it from your Angular model/controller - Why prefix all your functions with
_
, except forinitialize
? - Hard coding
"http://localhost:1625/ocpu/library/imyApp/R/gen_cluster/json"
is bad - Production code should not have
console.log
- You are not using the variable
currentLayer
- You are not using the parameter
filters
in_getClusters
- You should not create a new listener function for each marker in
_setMarkers
, you could create 1 function before the loop with a reference tomap
. This might cause performance problems - You should merge
_toggleControls
and$(".mapControls .open").click(
, it does not look DRY
All in all, I find the Google Maps API unwieldy, and because of that I do not think this is too much. However, you do need to something with the configuration logic, right now it looks akward.