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

Leaflet proj refactor #126

Merged
merged 34 commits into from
Jan 25, 2017
Merged

Leaflet proj refactor #126

merged 34 commits into from
Jan 25, 2017

Conversation

perliedman
Copy link
Contributor

Leaflet 1.0 is out since a couple of weeks, and there's no reason Proj4Leaflet's master branch should use 0.7.

perliedman and others added 29 commits November 29, 2013 14:44
Leaflet proj refactor (Leaflet beta 1.0.0)
When you try to create a L.Proj.GeoJSON layer with no data (to add it
after layer creation):

        var layer = L.Proj.geoJson();
        layer.addData(geojson).addTo(map);

You get an exception:

        Uncaught TypeError: Cannot read property 'features' of undefined

Conflicts:
	src/proj4leaflet.js
update package.json to work with beta
Fixed zoom returning NaN if passed in scale bigger than scales defined
@theashyster
Copy link
Contributor

I agree.

Although there are quite a lot of conflicts to fix and the documentation also would need to be updated to reflect the changes and reference the Leaflet 0.7 users to the commit before the merge of this pull request. Or should there be a release for the current code in master targeting Leaflet 0.7 and then a release of the new code in master targeting Leaflet 1.0?

This looks quite big since master and leaflet-proj-refactor branches were a bit out of sync and should be tested well.

@perliedman
Copy link
Contributor Author

Yeah, the first commit in this PR is almost three years old 😺

I think it looks worse than it is, but yeah, this is going to require some review.

I forgotten when there was a 0.7 release last and if there's anything meaningful that has been done after that, but maybe @semone or @pthorin knows?

@semone
Copy link
Contributor

semone commented Oct 18, 2016

I don't think there has been any major changes or releases for 0.7, not that i can remember, no new festures.

I think we need to go through the docs since it has changed and uses gh-pages in master. I dont think its updated at all for 1.0. That is the only thing that i can recall

@theashyster
Copy link
Contributor

Yeah, true, :)

I agree on that also, for me it looked that quite a lot of things could have been and probably should have been merged into master already, since they were not Leaflet 1.0 specific, but seems that those 2 branches just started to live life on their own at one point.

If we take a look at https://github.com/kartena/Proj4Leaflet/releases then the 0.7.2 release was a year ago and since then there has been 6 commits to the master branch, not actually doing a lot with code itself 0.7.2...master So it looks like the updated documentation can also reference release 0.7.2 for those who are still working with Leaflet 0.7.x, and it is as @semone says, nothing noteworthy there.

As for 1.0.0-beta.2 release there are 16 commits since last year 1.0.0-beta.2...leaflet-proj-refactor so after fixing the documentation, conflicts and merging this PR in a stable release 1.0.0 would be really awesome to get out.

I think the documentation in leaflet-proj-refactor is pretty much updated for Leaflet 1.0.

@bhaskarvk
Copy link

Any progress on this ?

@pthorin
Copy link

pthorin commented Jan 25, 2017

I am going to work on this now, hopefully I will get through it today.

@pthorin pthorin merged commit 7a4216d into master Jan 25, 2017
<link rel="stylesheet" href="http://cdn.leafletjs.com/leaflet/v0.7.7/leaflet.css" />
</head>
<body>
<div id="map"></div>
<script src="http://cdn.leafletjs.com/leaflet/v0.7.7/leaflet.js"></script>
>>>>>>> master
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pthorin I saw that this is merged already, but looks like this was not fixed before merging.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed pressing save on that one! I'm working on trying to get automated tests so can fix that soon as well. Thanks for noticing!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) Heh, good. Great job!

@pthorin pthorin deleted the leaflet-proj-refactor branch January 26, 2017 09:05
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.

6 participants