Skip to content

Added Leaflet support and an example showing basic features #101

Merged
merged 19 commits into from Feb 20, 2012

8 participants

@palewire

This is in response to the discussion in issue #97. In the patch you can find a simple Leaflet plug-in that provides support for tile layers, popups, custom icons, polylines and other basic functions. As discussed in the issue, it remains undecided what to do for the default tile layer. I'd like to notify @pmascari, @freyfogle and @nrabinowitz about this pull request.

@palewire

After talking with @freyfogle, I've patched some fixes that he recommends which you can see in the commit directly above this message. I don't know the timeline for @mapstraction updates but, at the risk of being annoying, I'll drop some github mentions for @dezfowler or @ajturner incase you all have anything else they'd like to see happen with this pull request before it's considered for integration.

@ajturner
Mapstraction member

Thanks Ben - playing with this now.

@palewire

Excellent! Let me know if I can help.

@ajturner
Mapstraction member

I made a version of test/core.htm for testing Leaflet and it didn't work. For one, Leaflet requires a center and zoom to start and also a Layer. Then there are a few methods that aren't implemented (e.g. getMapType())

My opinion is that a user should be able to switch to Leaflet with no code modifications. This would mean having to have defaults if none were specified on startup (layer, zoom). I realize this may be then odd for developers that want to override it but it's the reason they're using Mapstraction as a complete wrapper and not just Mapstraction as a set of method name changes.

How do you think we can handle the defaults so that it's just changing 'googlev3' to 'leaflet' in the test file (and including the JS)

@palewire

Hmm. Off the top of my head, I think it would not only require picking a tile provider as the default, but also picking layers to substitute for each map type in googlev3. Looks like it offers four layers: roads, satellite, hybrid and terrain.

Leaflet is a product of CloudMade, so using their tiles might be a natural choice, but their system requires an API key, which we probably shouldn't bake into the library. So that's out, IMHO. Looking at the OpenLayers mapstraction code, I only see one OpenStreetMap layer, so that doesn't point the way. MapQuest seems to offer two OSM based layers, roads and aerial, which would get you about half of what Google offers, but at least gives you something distinct to flop between and could substitute in for mxn.Mapstraction.ROAD and mxn.Mapstraction.SATELLITE.

As far as a center and zoom default, I would pitch just using 0,0 off the coast of Africa since it's an easy-to-understand default that is used by other libraries like OpenLayers.

As far as overriding the default layers, I'm not sure what the best route is. I haven't seen it anywhere else in the library, so I'm not sure if there's a style already developed. But maybe it could involve passing an option to the constructor that tells it to boot without a layer. Though if you substitute a layerset that doesn't correspond to the getMapType code (i.e. say, you provide only one layer when it's crafted to switch between two), it could crash when you run that. So you would probably need getMapType to somehow dynamically generate its options based on the currently loaded layers.

What do you think? Also, I'll tag @freyfogle since I know he has opinions about this stuff.

@palewire

Any thoughts? You want me to go ahead and try these fixes?

@ajturner
Mapstraction member

I was hoping someone else would chime in.

As you pointed out - OpenLayers defaults to OSM so it "just works" by flipping the provider and not requiring any keys. Setting 0,0 as the default is fine.

The one issue with this path is that if someone wants to make a good experience by loading immediately into a specific area, the OSM basemap and Zoomed area will possibly appear first before the new setCenter and Zoom and basemap are set. These could be provided as optional settings in the constructor to specify a Location and Zoom level, as well as you point out a basemap.

so for example:

var map = new Mapstraction("map_id", "leaflet",  {
   center: [-122,54], 
   zoom: 8, 
   tilelayer: "http://{s}.tile.cloudmade.com/YOUR-API-KEY/997/256/{z}/{x}/{y}.png"
})

or even make

tilelayer: {
   url:"http://tile.openstreetmap.org/{Z}/{X}/{Y}.png", 
   opacity: 1.0, 
   copyright: "OSM", 
   min_zoom:1, 
   max_zoom: 19, 
   selectable: true
}

we could move the debug variable as an key into that object literal

@freyfogle
Mapstraction member

Hi guys,

sorry for my silence. Have simply been swamped with other things.

three comments,

  1. don't let perfect be the enemy of good

  2. I would use mapquest for the example. No key needed, is OSM data, and will hold up if people are lazy and just cut and paste and their app gets ton of use.

  3. for initial coordinates for examples I advocate 51.513286, -0.136626. I think 0,0 is not great since it might lead people to think there's an error since they don't see anything (ie just ocean).

sorry I'm not of more help, am just hammered right now.

@dezfowler
Mapstraction member
@palewire

Okie dokie. I took a stab here at using MapQuest as a default tile set, and then integrating both its ROAD and SAT layers to work with setMapType and getMapType. Take a look @ajturner and @freyfogle and let me know if you think we're going in the right direction.

@palewire

FYI, we've got some new patches provided by @samiljan.

@freyfogle freyfogle merged commit 2831f92 into mapstraction:master Feb 20, 2012
@freyfogle
Mapstraction member
@pyxis777

I was getting the error "iconUrl not set in options" and found the fix here: Leaflet/Leaflet#826

Mapstraction version 2.0.18 has not updated its leaflet module to include the iconUrl updates so I had to change lines 281-285 of mxn.leaflet.core.js (toProprietary function) because leaflet changed the Marker definition.

OLD:

if (me.iconUrl) {
thisIcon = thisIcon.extend({
iconUrl: me.iconUrl
});
}

NEW:

if (me.iconUrl){
thisIcon = thisIcon.extend({
options: {
iconUrl: me.iconUrl
}
});

@vicchi
Mapstraction member
vicchi commented Sep 18, 2013

Thanks for the heads-up. Mapstraction v2.0.18 is no longer being actively maintained; all efforts are focused on the forthcoming v3.0.0 release. This bug in Leaflet has already been fixed on the release-3.0 branch as a result of Pull Request #213 in March of this year. See this commit for the details - palewire@85062f0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.