Skip to content

Instantly share code, notes, and snippets.

@rclark rclark/map.js
Created Oct 29, 2013

Embed
What would you like to do?
little code review
function onEachFeature(feature, layer) {
/* From the perspective of reading this function, it would be easier if you think about it in a linear fashion
What does this function do?
1) get a single feature
2) parse some specific properties out of the feature
3) use those properties to generate a string of HTML
4) build a popup from that HTML
Right now its a little hard to read because you're jumping around between those steps a bit
*/
// Step #2 Gather properties from the feature:
// One `var`, many variables, comma-separated
var pdf = feature.properties.Pdf.split(','), // keep the data simple, make the app "more complex"
label = feature.properties.Label,
bbox = layer.getBounds().toBBoxString(),
/*
This whole function can be boiled down to pretty much a single `.map` function, where you take the last result
But aside from that, there are some things to think about in how you wrote this one:
var preview = function() {
this.path; << Why `this`? If it doesn't do anything for you, don't use it.
_.each(pdf, function(item){
this.path = 'assets/' + item + '.png';
});
return this.path;
}
*/
preview = _.chain(pdf) // see http://underscorejs.org/#chain
.map(function (item) { return 'assets/' + item + '.png'; }) // get an array of URLs
.last() // take the last one
.value(), // return the "chain's value": http://underscorejs.org/#value
/* This is another one that boils down to an array.map function. Always use array.map if your
goal is to take one array, process each item in it, and end up with another array
var download_path = function(item) {
base_uri = document.baseURI.toString(); <<< I don't think you need to make absolute URLs here
whole_uri = base_uri.substring(0, base_uri.lastIndexOf("/") + 1);
return whole_uri + item + '.pdf'
}
var links = function() {
this.html; << the use of `this` here makes things really confusing
var template = _.template('<ul><a href= <%= path %>> <%= name %></a></ul>'); <<< this is so simple that you don't really need templating helper
_.each(pdf, function(item) {
path = download_path(item);
if (pdf.length > 1) {
this.html += template({path: path, name: item});
} else {
this.html = template({path: path, name: item});
}
});
return this.html;
}
*/
_.each([], function () {})
[].forEach(funtion () {})
[].map(function () {})
links = _.map(pdf, function (item) {
var path = item + '.pdf';
return '<li><a href="' + path + '">' + item + '</a></li>';
}).join('');
/* There's no need for a dedicated function here, because you're only going to call it once.
var template = function(title, preview, links, bbox) {
return '<div class=title><h4>' + title + ' Study Area</h4></div>' +
'<div class=content>' +
'<div class=preview><img src=' + preview + '></div>' +
'<div class=links><h6>Downloadable Maps:</h6>' +
'<div class=download>' + links + '</div>' +
'<div id=zoom>' +
'<button type="button" onclick="doZoom(' + bbox + ')" class="btn btn-success">Zoom to this study area</button>' +
'</div>' +
'</div>' +
'</div>';
}
*/
// Step #3 Turning the feature into a string of html
// I'm starting a new `var` here just for clarity. This one is a complex bit of content
var html = '<div class=title><h4>' + label + ' Study Area</h4></div>';
html += '<div class=content>';
html += '<div class=preview><img src=' + preview + '></div>';
html += '<div class=links><h6>Downloadable Maps:</h6>';
html += '<div class=download><ul>' + links + '</ul></div>';
html += '<div id=zoom>';
html += '<button type="button" onclick="doZoom(' + bbox + ')" class="btn btn-success">Zoom to this study area</button>';
html += '</div></div></div>';
// Step #4 Create a popup and bind it to the layer
var popup = L.popup({
'maxWidth': 500,
'minWidth': 250
}).setContent(html);
layer.bindPopup(popup);
}
var doZoom = function (bbox0, bbox1, bbox2, bbox3) {
//var sw = new L.LatLng(bbox1, bbox0);
//var ne = new L.LatLng(bbox3, bbox2);
//var bounds = new L.LatLngBounds(sw, ne);
// Little shortcut here
var bounds = L.latLngBounds([[bbox1, bbox0], [bbox3, bbox2]]);
app.map.fitBounds(bounds);
}
// I understand the effeciency here, but it takes another step to understand what's happening = harder to read
// When there are so few layers, I'd say just add them
/*
for (var key in app.layers) {
app.layers[key].addTo(app.map);
}
*/
app.layers.baseLayer.addTo(app.map);
app.layers.fissuresLayer.addTo(app.map);
app.layers.studyAreas.addTo(app.map);
L.geocoderControl().addTo(app.map);
d3.json('data/studyareas.json', function (err, content) {
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.