Mappa icon indicating copy to clipboard operation
Mappa copied to clipboard

Leaflet.js total refactor!

Open GoToLoop opened this issue 7 years ago β€’ 3 comments

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

  1. 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?

  2. 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!

  3. 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!!!

GoToLoop avatar Feb 09 '18 19:02 GoToLoop

Codecov Report

Merging #10 into master will decrease coverage by 0.22%. The diff coverage is 0%.

Impacted file tree graph

@@            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.

codecov-io avatar Feb 09 '18 19:02 codecov-io

wow, this is amazing! thanks for the PR!

I left some comments, but regarding your points:

  1. Perfect. Makes total sense.

  2. Seems good to me, I just left some questions on the review.

  3. 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

cvalenzuela avatar Feb 09 '18 23:02 cvalenzuela

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.

ChaimStanton avatar Apr 08 '22 04:04 ChaimStanton