search

Leaflet / Leaflet.VectorGrid

Display gridded vector data (sliced GeoJSON or protobuf vector tiles) in Leaflet 1.0.0
598 stars 194 forks source link

remove 'getLatLng' property from PointSymbolizer - references #148 #157

Closed chriszrc closed 5 years ago

chriszrc commented 6 years ago

The error is visible on the official vectorgrid demo pages:

http://leaflet.github.io/Leaflet.VectorGrid/demo-points-icons.html

Just open the console and mouse over any of the markers. The error precludes any mouseover or click events on point features.

My Diagnosis: Point Symbolizer instances inherit from marker or circlemarker, but they lack latlng information, and on mouseover or click, leaflet will error on these features if it finds the getLatLng method, since that indicates it should be treated as a real marker instance, but even though the method is there, the value isn't, so leaflet produces an error.

Maybe these instance should have a latlng, I don't know, this was just an easy fix so that I could continue to use vectorgrid protobuf support with points and have mouseover and click events work.

tomchadwin commented 6 years ago

My gut feeling is that there should be a latlng.

chriszrc commented 6 years ago

I fully agree, but I read through all the source code, and for the life of me, could not figure out how to add that. As I said, I definitely don't know if this is the right answer, but also wasn't getting any traction in the issues section, so I tried to put forth some good faith effort here-

tomchadwin commented 6 years ago

Absolutely - I have the same position with a different issue/PR. I just meant that this is probably fine for your fork, but that we need LatLng fixed here. Not trying to belittle your work one bit.

tomchadwin commented 5 years ago

Dredging this back up again because of requests in #148 that this be merged, here's an attempt at understanding this better:

If this summary is correct, we need the conversion method, and we need to reimplement CircleMarker.getLatLng() to use that new method.

Is that right?

chriszrc commented 5 years ago

@perliedman ok, I'll make those changes-

bagage commented 5 years ago

Hello @chriszrc, any news on this? Thanks!

chriszrc commented 5 years ago

@perliedman Ok, I think I backed out the changes to dist and whitespace, let me know if there's anything else I can do to get this change merged-

perliedman commented 5 years ago

@chriszrc nice! It still looks like the dist/ files are part of the PR, though - they all show up under "Files changed". Also, looks like some unrelated changes to package-lock.json snuck in there, just get rid of that changes as well, and I think we are good to go!

chriszrc commented 5 years ago

Ok, I'm looking at this page now:

https://github.com/Leaflet/Leaflet.VectorGrid/pull/157/files

And it seems like maybe I got everything this time-

perliedman commented 5 years ago

Nice! Thanks for all the fixes, great!

bagage commented 5 years ago

Any chance to publish a new version on npm with this fix? Thanks!

m311ow commented 5 years ago

Hi, i came across the mentioned bug recently, and tried to fixed it by replacing leaflet.vectorgrid in package.json with @chriszrc fork, then i tried to run yarn install inside node_modules/leaflet.vectorgrid since i noticed that dist folder is missing, however i was unable to make it work because of errors in runas build (runas is one of dependencies)....Does anyone have similiar experience or can at least point me in right direction plz ? Im on linux btw

some lines from error message that could be of some use :