Tilemap refactor #3445
Tilemap refactor #3445
Conversation
…ixels to meters for circle markers, removed resizeFeatures method
…setting and url for explanation
… width or height of geohash grid, cleaned up some extraneous code
… into tilemap-refactor
|
Can you add a detailed description of each bug fix and enhancement and also link this back to the issues with "closes #blah" |
| var ne = L.latLng([gh[2][1], gh[2][0]]); | ||
| var sw = L.latLng([gh[0][1], gh[0][0]]); | ||
| var ew = Math.floor(nw.distanceTo(ne)); | ||
| var ns = Math.floor(nw.distanceTo(sw)); |
There was a problem hiding this comment.
two letter variables are hard to read, can you expand these to be descriptive of their purpose?
|
There's an error as soon as I create a tilemap vis, on tile_map.js, line 121 - looks like a missing DOM element?
|
|
@w33ble misunderstood your previous example. This behaves the same as master now. |
| return L.circleMarker(latlng, { | ||
| radius: rad | ||
| }); | ||
| var gh = feature.properties.rectangle; |
There was a problem hiding this comment.
gh is a pretty bad name, I have no idea what it's short for. Also, a note about what's going on here would be helpful, in particular, why [0,0], [0,1], [2,1], [2,0].
|
Aside from the code syntax comments, this seems functional and looks good to me. Passing to @panda01 just to get another set of eyes on it, since there's a lot here to review. |
|
|
||
| // super min and max from all chart data | ||
| var min = mapData.properties.allmin; | ||
| var max = mapData.properties.allmax; |
There was a problem hiding this comment.
Hmm, it doesn't appear to me like these variables are used.
|
@w33ble @panda01 thanks for the review and feedback. I know there were too many commits in there. I have updated the PR to address your comments:
|
| var self = this; | ||
|
|
||
| var length = mapData.properties.length; | ||
| var precision = mapData.properties.precision; |
There was a problem hiding this comment.
It appears to me that these variables aren't used in this function.
There was a problem hiding this comment.
@panda01 thanks. Fixed this one.
…that was used in three places
Includes several 'low-hanging fruit' updates: