search

cvalenzuela / Mappa

A canvas wrapper for Maps 🗺 🌍
https://mappa.js.org
359 stars 104 forks source link

Leaflet.js total refactor! #10

Open GoToLoop opened 6 years ago

GoToLoop commented 6 years ago

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/leaflet@1.3.0/dist/leaflet.js' to 'https://unpkg.com/leaflet@1.3/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!!!

codecov-io commented 6 years ago

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.

cvalenzuela commented 6 years ago

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

ChaimStanton commented 2 years ago

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.