stamen / modestmaps-js

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

Auto size #95

Closed tmcw closed 12 years ago

tmcw commented 12 years ago

This changes the autoSize variable into a getter/setter function. As I noted in #89, setSize behavior changed significantly in the pre-1.0.0 work, and ended up in a spot where some users would need to reimplement setSize externally.

This turns the autoSize manipulation added in setSize into a public method: map.autoSize() - true or false to add/remove the listeners and no arguments to get.

So pre-merge behavior was:

map.setSize(500, 500);
// map continues to be autosized

post merge is:

map.setSize(500, 500);
// map can't go back to being autosized

and this pull allows for

map.setSize(500, 500);
map.autoSize(false);
// map is no longer autosized

or

map.setSize(500, 500);
// map continues to be sized

I think that this behavior is more predictable, since set size should 'just' set size and not manipulate map behavior beyond that, but it could be reversed easily.

RandomEtc commented 12 years ago

I'm not totally sold on this - why would you want autoSize to true and also be calling setSize? It should be one or the other.

tmcw commented 12 years ago

Fullscreen toggles: see this commit - https://github.com/mapbox/wax/commit/7b6392fefe7d4c8d9ad6c00892e63b9b42da3cb5#L1L27

Basically if you want to have two states that a map can have, of different sizes, and one or both of them can change with window size.

RandomEtc commented 12 years ago

I see, that makes sense. It might be simpler to make draw() update dimensions every frame if autoSize is set? It's probably a premature optimization to cache it as we do now.

As introduced, the autoSize property was intended to be an internal variable... if it's becoming a method in the public/documented API it should probably have a setAutoSize(val)/getAutoSize() method pair as well as a verb free version.

tmcw commented 12 years ago

Fixed in #89