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

Make that L.Toolbar inherits from L.Layer if it exists #21

Merged
merged 2 commits into from Aug 31, 2015

Conversation

yohanboniface
Copy link
Member

This adds minimal compat with Leaflet master.

Thanks :)

@yohanboniface
Copy link
Member Author

It's failing on #22

@justinmanley
Copy link
Member

@yohanboniface - Could you explain a bit more?

Where is the need to inherit from L.Layer coming from?

@yohanboniface
Copy link
Member Author

Sure :)

The Leaflet mastermap.addLayer expects the added layer to have a _layerAdd method, which, in the inheritance toolchain comes from L.Layer (and which is responsible to call the onAdd method then).

So I thought that, as soon as we use map.addLayer, we should conform to the inheritance toolchain.

Given that the toolbar is not really a layer but more of a control, maybe a stronger fix in the future would be to stop using addLayer, but I'm ensure how we can support both 0.7 and master this way.

What do you think?

@yohanboniface
Copy link
Member Author

ping @manleyjster :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 93.65% when pulling a549ee6 on yohanboniface:leaflet1.0-compat into fad64a3 on Leaflet:master.

@justinmanley
Copy link
Member

@yohanboniface - Will this PR make a difference to you immediately (i.e. are you currently using Leaflet master?), or are you anticipating future issues that will arise once Leaflet 1.0 is released?

If this PR is anticipatory, I'd rather hold off on merging it for the time being, since I expect that there will be a great many changes to be made once Leaflet 1.0 comes out. I'd rather wait to work on 1.0 compatibility until the state of the library is settled and we can look at the whole pictures.

This is great, though, because it's getting the discussion started!

@yohanboniface
Copy link
Member Author

A bit of both ;)

We are close to the 1.0 beta version: https://github.com/Leaflet/Leaflet/milestones/1.0-beta1
And I'm starting porting uMap on Leaflet 1.0: https://github.com/yohanboniface/Leaflet.Storage/issues/94 and I want to switch to Leaflet.Toolbar in the meantime (because it needs more interactions for multipolgon/polyline support, and I want to use the popup mode :) ).

So as you prefer, I can maintain a Leaflet1.0 branch on my own fork in the meantime also. But I think such a huge milestone (Leaflet 1.0) needs to be done also by the community, porting plugins one by one before the release, so we can have feedback and bug report before issuing the release. For sure, you can wait for the first official beta. But given that those changes are both 0.7 and 1.0 compliant, I would merge, to help pushing the community forward, and help people that start migrating by anticipation ;)

Anyway, your choice :)

@yohanboniface
Copy link
Member Author

@mourner
Copy link
Member

mourner commented Aug 3, 2015

Since the changes are minimal, I'd say let's rebase and merge into master.

mourner added a commit that referenced this pull request Aug 31, 2015
Make that L.Toolbar inherits from L.Layer if it exists
@mourner mourner merged commit 2ab2b88 into Leaflet:master Aug 31, 2015
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.

None yet

4 participants