search

stamen / modestmaps-js

Modest Maps javascript port
http://modestmaps.com
566 stars 152 forks source link

A quick fix for getExtent #111

Closed nateirwin closed 12 years ago

shawnbot commented 12 years ago

Thanks, Nate!

tmcw commented 12 years ago

What was the bug that this was fixing?

nateirwin commented 12 years ago

The MM.Extent constructor expects four arguments: north, west, south, and east. It was being passed two MM.Location objects, so south and east were undefined.

tmcw commented 12 years ago

I think the bug is in the Extent constructor itself, though - look at https://github.com/stamen/modestmaps-js/blob/master/src/extent.js#L7 and there's the code for handling the two-location case, but it should probably early-return to avoid running the rest.

nateirwin commented 12 years ago

I agree that there is a bug in the Extent constructor, but I would also say that modifying getExtent to make it pass the proper arguments to the constructor is best.

I did a quick browse through the source, and I couldn't find anywhere internally where an MM.Extent object is created by passing two MM.Location objects into the constructor.

Now if that needs to be maintained to ensure backwards-compatibility that's another story altogether.

tmcw commented 12 years ago

There was an upstream logic error in the MM.Extent code, which I fixed in 25cfffb659c4bce57359014c2a935058ea52f2f0