Update to current leaflet. #8

Closed
tmcw opened this Issue Jun 19, 2014 · 11 comments

Comments

Projects
None yet
5 participants
@tmcw
Contributor

tmcw commented Jun 19, 2014

No description provided.

@nikolauskrismer

This comment has been minimized.

Show comment
Hide comment
@nikolauskrismer

nikolauskrismer Jun 3, 2015

Oh... that would be great.
I just tries your plugin with leaflet from today's master brunch. I get a "invalid instanceof operand" error...

BTW: Thanks for that great leaflet plugin :-)

Oh... that would be great.
I just tries your plugin with leaflet from today's master brunch. I get a "invalid instanceof operand" error...

BTW: Thanks for that great leaflet plugin :-)

@r-barnes

This comment has been minimized.

Show comment
Hide comment
@r-barnes

r-barnes Jul 16, 2015

I am getting Uncaught TypeError: Expecting a function in instanceof check, but got undefined when using leaflet-pip with 1.0-dev and strict JS. This may be related, but I'll start a new thread.

I am getting Uncaught TypeError: Expecting a function in instanceof check, but got undefined when using leaflet-pip with 1.0-dev and strict JS. This may be related, but I'll start a new thread.

@tmcw

This comment has been minimized.

Show comment
Hide comment
@tmcw

tmcw Jul 16, 2015

Contributor

Patches accepted, as always, if someone wants to help with this update.

Contributor

tmcw commented Jul 16, 2015

Patches accepted, as always, if someone wants to help with this update.

@r-barnes

This comment has been minimized.

Show comment
Hide comment
@r-barnes

r-barnes Jul 18, 2015

I do like to contribute patches! But this is somewhat outside my
expertise. Fortunately, I was able to get things running by disabling
the MultiPolygon check. Thanks again for building this!

Cheers,
Richard

On 07/16/2015 12:19 PM, Tom MacWright wrote:

Patches accepted, as always, if someone wants to help with this update.


Reply to this email directly or view it on GitHub
#8 (comment).

I do like to contribute patches! But this is somewhat outside my
expertise. Fortunately, I was able to get things running by disabling
the MultiPolygon check. Thanks again for building this!

Cheers,
Richard

On 07/16/2015 12:19 PM, Tom MacWright wrote:

Patches accepted, as always, if someone wants to help with this update.


Reply to this email directly or view it on GitHub
#8 (comment).

@nikolauskrismer

This comment has been minimized.

Show comment
Hide comment
@nikolauskrismer

nikolauskrismer Jul 21, 2015

Ok... the API of leaflet for 1.0 changes.
At the moment (talking of beta1) there is no L.MultiPolygon anymore.
Since leaflet-pip performs a

if (l instanceof L.MultiPolygon || l instanceof L.Polygon)

check, this is a problem.

For now the solution would be to only check for L.Polygon (since now MultiPolygons are handled also in this class), but the leaflet API might change again (see Leaflet/Leaflet#3498 (comment))

Ok... the API of leaflet for 1.0 changes.
At the moment (talking of beta1) there is no L.MultiPolygon anymore.
Since leaflet-pip performs a

if (l instanceof L.MultiPolygon || l instanceof L.Polygon)

check, this is a problem.

For now the solution would be to only check for L.Polygon (since now MultiPolygons are handled also in this class), but the leaflet API might change again (see Leaflet/Leaflet#3498 (comment))

@nikolauskrismer

This comment has been minimized.

Show comment
Hide comment
@nikolauskrismer

nikolauskrismer Jul 21, 2015

How about opening a separate branch for leaflet-1.0-dev?

How about opening a separate branch for leaflet-1.0-dev?

@r-barnes

This comment has been minimized.

Show comment
Hide comment
@r-barnes

r-barnes Jul 31, 2015

This is the only breaking change I experienced in using leaflet-pip with Leaflet 1.0, so the fix seems easy enough: just remove the line. Except then things don't work for earlier versions of Leaflet. To make matters worse, the discussion on Leaflet regarding MultiPolygon is on-going

A branch would work, or a comment could be appended to the source code referencing the relevant threads so users can confidently repair the issue themselves until the API stabilizes.

This is the only breaking change I experienced in using leaflet-pip with Leaflet 1.0, so the fix seems easy enough: just remove the line. Except then things don't work for earlier versions of Leaflet. To make matters worse, the discussion on Leaflet regarding MultiPolygon is on-going

A branch would work, or a comment could be appended to the source code referencing the relevant threads so users can confidently repair the issue themselves until the API stabilizes.

@MuellerMatthew

This comment has been minimized.

Show comment
Hide comment
@MuellerMatthew

MuellerMatthew Aug 5, 2015

Why not just amend the code so it checks if L.MultiPolygon exists, and if it does, it uses it, otherwise it wont. That way when the code changes, it will still work. perhaps something like the following:

add a new function to handle both situations:

instanceCheck: function() {
     if (L.MultiPolygon) {
          return (l instanceof L.MultiPolygon || l instanceof L.Polygon);
     } else {
          return(l instanceof L.Polygon);
     }
}

and then replace

if (l instanceof L.MultiPolygon || l instanceof L.Polygon)

with

if (instanceCheck())

fyi, I havn't verified whether this works, but in theory, it should.

Why not just amend the code so it checks if L.MultiPolygon exists, and if it does, it uses it, otherwise it wont. That way when the code changes, it will still work. perhaps something like the following:

add a new function to handle both situations:

instanceCheck: function() {
     if (L.MultiPolygon) {
          return (l instanceof L.MultiPolygon || l instanceof L.Polygon);
     } else {
          return(l instanceof L.Polygon);
     }
}

and then replace

if (l instanceof L.MultiPolygon || l instanceof L.Polygon)

with

if (instanceCheck())

fyi, I havn't verified whether this works, but in theory, it should.

@nikolauskrismer

This comment has been minimized.

Show comment
Hide comment
@nikolauskrismer

nikolauskrismer Oct 28, 2015

Why use instanceof here?
Wouldn't the following thing make more sense (especially since we need l.toGeoJSON().geometry anyway):

var leafletPip = {
    bassackwards: false,
    getGeometry: function(l) {
        var geom = null,
            type = null;

        if (l.toGeoJSON) {
            geom = l.toGeoJSON().geometry;
            type = (geom) ? geom.type : null;

            // checking for supported types (only MultiPolygon and Polygon)
            if (type === "MultiPolygon" || type === "Polygon") {
                return geom;
            }
        }

        return null;
    },

    pointInLayer: function(p, layer, first) {
        'use strict';
        if (p instanceof L.LatLng) p = [p.lng, p.lat];
        else if (leafletPip.bassackwards) p = p.concat().reverse();

        var results = [];
        layer.eachLayer(function(l) {
            if (first && results.length) return;
            var geom = leafletPip.getGeometry(l);
            if (geom && gju.pointInPolygon({ type: 'Point', coordinates: p }, geom)) {
                results.push(l);
            }
        });

        return results;
    }
};

Why use instanceof here?
Wouldn't the following thing make more sense (especially since we need l.toGeoJSON().geometry anyway):

var leafletPip = {
    bassackwards: false,
    getGeometry: function(l) {
        var geom = null,
            type = null;

        if (l.toGeoJSON) {
            geom = l.toGeoJSON().geometry;
            type = (geom) ? geom.type : null;

            // checking for supported types (only MultiPolygon and Polygon)
            if (type === "MultiPolygon" || type === "Polygon") {
                return geom;
            }
        }

        return null;
    },

    pointInLayer: function(p, layer, first) {
        'use strict';
        if (p instanceof L.LatLng) p = [p.lng, p.lat];
        else if (leafletPip.bassackwards) p = p.concat().reverse();

        var results = [];
        layer.eachLayer(function(l) {
            if (first && results.length) return;
            var geom = leafletPip.getGeometry(l);
            if (geom && gju.pointInPolygon({ type: 'Point', coordinates: p }, geom)) {
                results.push(l);
            }
        });

        return results;
    }
};
@smitchell

This comment has been minimized.

Show comment
Hide comment
@smitchell

smitchell Dec 17, 2015

I applied the code from nikolauskrismer, and it got me up and running with Leaflet 1.0 Beta 2... mostly. The states in my demo, http://exploringspatial.com/#demo/8, won't unhighlight any more. I'm going to see if that is an easy fix (at this point, I'm working on a local copy).

I applied the code from nikolauskrismer, and it got me up and running with Leaflet 1.0 Beta 2... mostly. The states in my demo, http://exploringspatial.com/#demo/8, won't unhighlight any more. I'm going to see if that is an easy fix (at this point, I'm working on a local copy).

@tmcw

This comment has been minimized.

Show comment
Hide comment
@tmcw

tmcw May 26, 2016

Contributor

Fixed in #20

Contributor

tmcw commented May 26, 2016

Fixed in #20

@tmcw tmcw closed this May 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment