Leaflet.js total refactor! #10

GoToLoop · 2018-02-09T19:20:12Z

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

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 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()**. Well, I've refactored it to access _this.canvas_ as a closure variable simply called _canvas_ there. 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.

codecov-io · 2018-02-09T19:25:26Z

Codecov Report

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

@@            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%)`	⬇️

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

Looks good! I just left a few comments mostly on the style

cvalenzuela · 2018-02-09T22:44:04Z

src/providers/tile/Leaflet.js

- this.loadSrc();
- }
+
+ this.constructor.name === 'Leaflet' && this.loadSrc();


I am using regular if statements as a default instead of ternary or short-circuit evaluations. Just as a style guide.

cvalenzuela · 2018-02-09T22:51:49Z

src/providers/tile/Leaflet.js

- overlayPane.appendChild(container);
+ L.Layer.Overlay = L.Layer.extend({
+ onAdd(map) {
+ this._container = L.DomUtil.create('div', 'leaflet-layer');


Is there any need to have _container as class property? can it be just const container?

As I've stated, I'm following the tutorial from here:
http://LeafletJS.com/examples/extending/extending-2-layers.html

Property _container is needed for implementing L.Layer::onRemove(), so it knows which HTMLElement, which is created in L.Layer::onAdd(), to remove.

And according to Leaflet.js' API, L.Layer::onRemove() isn't optional, just like L.Layer::onAdd():
http://LeafletJS.com/reference-1.3.0.html#layer-onremove

Besides, _container gets delete later on inside L.Layer::onRemove() anywayz. ^_^

Just 1 extra observation: property _container is being temporarily added to the L.Layer subclass, not Leaflet.

Recall that L.Layer::onAdd() isn't a fat arrow function anymore, and its this isn't bound to Leaflet now.

And b/c the subclass of L.Layer isn't exposed, getting access to L.Layer::_container isn't that easy.

Although not impossible. I think that calling L.Map::eachlayer() can work for example: :D
http://LeafletJS.com/reference-1.3.0.html#map-eachlayer

cvalenzuela · 2018-02-09T22:56:29Z

src/providers/tile/Leaflet.js

- const cnvs = this.canvas.getContext('webgl') || this.canvas.getContext('2d');
+ L.overlay = () => new L.Layer.Overlay;
+
+ this.tiles && (this.tiles.options.opacity = this.options.opacity);


(just a note on replacing the short-circuit evaluations)

cvalenzuela · 2018-02-09T22:57:20Z

src/providers/tile/Leaflet.js

- };
- })
+ const d = this.map.dragging._draggable._newPos;
+ d && (ctx.canvas.style.transform = `translate(${-d.x}px, ${-d.y}px)`);


same as above

cvalenzuela · 2018-02-09T22:58:16Z

src/providers/tile/Leaflet.js

- return this.map.getZoom();
- }
- return 0;
+ return this.ready? this.map.getZoom() : 0;


Well, that's 4 lines shortened into 1 short statement. RU sure do you prefer the 4-line fatty back here?

I'm using eslint with the airbnb style guide and that defaults to no-ternary. I previously had everything written in ternary but at some point, it got messier to manage, so I switch everything to maintain consistency. Like here.

Still everything gets minified in prod so all good

Oh, that's a pity. Not advocating big multi-line ternaries; rather strategic 1s which is still clear & short like this 1.
However, if you've got such very strict eslint rule in place... well, let's go back to the ugly 4-line boilerplate. :P

cool! thanks

cvalenzuela · 2018-02-09T23:00:28Z

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

Make **fromPointToLatLng()** return a stripped down raw { lat, lng } just like **fromLatLngToPixel()**.

ChaimStanton · 2022-04-08T04:22:20Z

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.

GoToLoop added 2 commits February 9, 2018 17:14

Update Leaflet.js

0b61d49

Update Leaflet.js

5a1d4d0

cvalenzuela reviewed Feb 9, 2018

View reviewed changes

GoToLoop added 8 commits February 9, 2018 21:14

Style Revert

1f3b559

Ternary removed!

b016036

(1.3.x)

2667d9c

fromPointToLatLng()

1b01461

Make **fromPointToLatLng()** return a stripped down raw { lat, lng } just like **fromLatLngToPixel()**.

Update Leaflet.js

18f580f

canvasOverlay() -> onAdd() -> on()

95c02f9

Update Leaflet.js

36077ab

this.map = this.tiles = null;

db335c5

ChaimStanton mentioned this pull request Apr 8, 2022

Uncaught type error when I attempt to redraw the map and overlay after deleting it. #47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leaflet.js total refactor! #10

Leaflet.js total refactor! #10

GoToLoop commented Feb 9, 2018

codecov-io commented Feb 9, 2018 •

edited

Loading

cvalenzuela left a comment

cvalenzuela Feb 9, 2018

cvalenzuela Feb 9, 2018

GoToLoop Feb 9, 2018 •

edited

Loading

GoToLoop Feb 9, 2018 •

edited

Loading

cvalenzuela Feb 9, 2018

GoToLoop Feb 10, 2018 •

edited

Loading

cvalenzuela Feb 9, 2018

cvalenzuela Feb 9, 2018

cvalenzuela Feb 9, 2018

GoToLoop Feb 9, 2018

cvalenzuela Feb 9, 2018 •

edited

Loading

GoToLoop Feb 9, 2018 •

edited

Loading

cvalenzuela Feb 9, 2018

cvalenzuela commented Feb 9, 2018

ChaimStanton commented Apr 8, 2022

Leaflet.js total refactor! #10

Are you sure you want to change the base?

Leaflet.js total refactor! #10

Conversation

GoToLoop commented Feb 9, 2018

codecov-io commented Feb 9, 2018 • edited Loading

Codecov Report

cvalenzuela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoToLoop Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

GoToLoop Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoToLoop Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvalenzuela Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

GoToLoop Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvalenzuela commented Feb 9, 2018

ChaimStanton commented Apr 8, 2022

codecov-io commented Feb 9, 2018 •

edited

Loading

GoToLoop Feb 9, 2018 •

edited

Loading

GoToLoop Feb 9, 2018 •

edited

Loading

GoToLoop Feb 10, 2018 •

edited

Loading

cvalenzuela Feb 9, 2018 •

edited

Loading

GoToLoop Feb 9, 2018 •

edited

Loading