Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

L.Proj.GeoJSON #32

Merged
merged 6 commits into from
Aug 19, 2013
Merged

L.Proj.GeoJSON #32

merged 6 commits into from
Aug 19, 2013

Conversation

sheppard
Copy link
Contributor

Hi, this pull request adds L.Proj.GeoJSON (and L.Proj.geoJson). Note that unlike the other Proj-aware layers, the CRS code is read from the JSON object directly, rather than being an argument to the constructor. This means it is not possible to set the Proj4js definition via the CRS constructor, so it must be done manually beforehand (see example).

One nice thing about this approach is that it makes it possible to display GeoJSON stored in a different projection than the map.

@perliedman
Copy link
Contributor

Thanks, this looks like a really nice addition.

I have some questions:

  1. What happens if you attempt to add a GeoJSON with a projection that has not been added in advance?
  2. Could you add a short example (more or less like you have in the docs) to the examples directory?

@sheppard
Copy link
Contributor Author

  1. It breaks - specifically, the GeoJSON is rendered without being reprojected, while Proj4js tries to asynchronously load the definition. I think it would make more sense to for Proj4js to throw an error in this case. One of the first tasks in the revamped proj4js project was to remove the async loading feature: First touchups proj4js/proj4js#1. Thus, it may make sense to document and punt on this issue until the next stable version of proj4js is released.
  2. Sure, I'll put something together.

@perliedman
Copy link
Contributor

I did not know about the work being done on modernizing Proj4js, so thanks for the heads up. It's definitely a project that needs some refactoring. The core functionality is very nice, but API and structure... not so much.

@sheppard
Copy link
Contributor Author

sheppard commented Aug 6, 2013

Ok, I added a complete example to the directory and further clarified the usage caveats. In order for the example to work, I needed to update Leaflet to 0.6 which adds the coordsToLatLng option to L.GeoJSON (see Leaflet/Leaflet#886, Leaflet/Leaflet#888 and Leaflet/Leaflet#1762). So this PR now has an additional purpose of updating lib/leaflet, let me know if that should be a separate step.

@sheppard
Copy link
Contributor Author

sheppard commented Aug 6, 2013

I also added a reference to geojson/draft-geojson#6 for completeness.

perliedman pushed a commit that referenced this pull request Aug 19, 2013
Adds L.Proj.GeoJSON for projected GeoJSON support.
@perliedman perliedman merged commit 72ee327 into kartena:master Aug 19, 2013
@perliedman
Copy link
Contributor

Thanks again. Sorry about the delay, I've been away for a couple of weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants