Mappa
Mappa copied to clipboard
Leaflet.js total refactor!
After taking a hard look at Leaflet.js (also TileMap.js) in order to find a more proper solution for some1 from the Processing's forum: https://Forum.Processing.org/two/discussion/26239/p5-js-mappa-js-unable-to-place-map-in-parent-div#Item_4
I've found out this class Leaflet got some tiny minor cosmetic issues. And that I could refactor it. :P
-
Current latest Leaflet.JS' version is now 1.3.1. But this class is loading 1.3.0 instead. So I've changed 'https://unpkg.com/[email protected]/dist/leaflet.js' to 'https://unpkg.com/[email protected]/dist/leaflet.js'.
@1.3
, rather than@1.3.0
, will automatically pick latest version from range v1.3.0 to v1.3.9. I don't think patch updates are that dangerous, are they? -
In canvasOverlay(), you've got an custom subclass from L.Layer named as L.overlay. However, according to JS/Java's conventions, class names should be capitalized. Therefore I've renamed L.overlay to L.Layer.Overlay. Then made a custom L.overlay() function which instantiates that just renamed subclass. ^_^ Given the now wrapper function got the same name as the previous subclass' name, no API call had been changed!
-
Besides that renaming and a new wrapper function in its place, I've found some instructions about extending class L.Layer below: http://LeafletJS.com/examples/extending/extending-2-layers.html However, your approach differs greatly from the article above. You use an arrow function for L.Layer::onAdd() method, permanently sealing its
this
to that of subclass Leaflet's. You do that in order to access this.canvas inside L.Layer::onAdd(), I know. Well, I've refactored it to access this.canvas as a closure variable simply called canvas there.
Well, those are my main changes. There are many more tiny 1s throughout the file though. :D
Got no idea how much you're gonna like them. Just tell me which 1s to revert back then.
P.S.: Not tested at all!!!
Codecov Report
Merging #10 into master will decrease coverage by
0.22%
. The diff coverage is0%
.
@@ Coverage Diff @@
## master #10 +/- ##
=========================================
- Coverage 44.52% 44.3% -0.23%
=========================================
Files 14 14
Lines 393 395 +2
=========================================
Hits 175 175
- Misses 218 220 +2
Impacted Files | Coverage Ξ | |
---|---|---|
src/providers/tile/Leaflet.js | 1.88% <0%> (-0.08%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Ξ = absolute <relative> (impact)
,ΓΈ = not affected
,? = missing data
Powered by Codecov. Last update 5a6a6d2...db335c5. Read the comment docs.
wow, this is amazing! thanks for the PR!
I left some comments, but regarding your points:
-
Perfect. Makes total sense.
-
Seems good to me, I just left some questions on the review.
-
I didn't know that! leaflet had a major released not so long ago and I haven't updated this repo to reflect that.
I will test it and let you know how it goes. I still haven't implemented all automatic tests so part of it will go manually.
Let me know your comments so we can move forward and merge
I think this commit may change the id attribute of the layer from what it was previously as #mappa to #leaflet. See my issue #47 for more info.