Skip to content

reconsider the way we specify controls on the map #273

Merged
merged 10 commits into from Mar 6, 2013

4 participants

@bartvde
OpenLayers member
bartvde commented Mar 5, 2013

I'm sure I've missed some previous discussions on this, but to me it feels a bit unnatural to have every control be specified as a boolean on the map config, and having its options also directly in the map's config. This really bloats the map config object and makes it hard to structure.

Why was it decided not to just pass in controls as a collection of controls like this was done in ol2? Because it requires re-specifying the default controls?

@twpayne
twpayne commented Mar 6, 2013

@bartvde, this is now ready for review.

@bartvde bartvde and 1 other commented on an outdated diff Mar 6, 2013
examples/epsg-4326.js
@@ -24,11 +25,15 @@ var layers = new ol.Collection([
]);
var map = new ol.Map({
+ controls: ol.control.defaults({
+ scaleLine: true,
+ scaleLineOptions: /** @type {ol.control.ScaleLineOptions} */ ({
@bartvde
OpenLayers member
bartvde added a note Mar 6, 2013

why are there parentheses around the scaleLineOptions object?

@twpayne
twpayne added a note Mar 6, 2013

Currently the typecast is required to pass the compiler. The compiler currently has difficulty with object literal types with optional properties. End-user code that does not use the compiler will not need the typecast.

@bartvde
OpenLayers member
bartvde added a note Mar 6, 2013

Thanks for the explanation @twpayne, this is gonna cause confusion though for our end users I fear. Examples is what they will look at first.
This morning @elemoine @bbinet and I discussed having Jasmine tests for the API and we also briefly discussed not having the compiler run on the examples, but then decided against it. But seeing this I think we should re-evaluate that decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine
OpenLayers member
elemoine commented Mar 6, 2013

This is great!

One comment: yesterday we said that the second arg to the defaults function would be a array, so I think it makes sense to stick to that.

Thanks!

@probins
probins commented Mar 6, 2013

would be nice to group interactions, so you can do touch:false or keyboard:false

@twpayne
twpayne commented Mar 6, 2013

would be nice to group interactions, so you can do touch:false or keyboard:false

Agree. I'll open an issue for this.

@twpayne twpayne merged commit 3a7c4bd into openlayers:master Mar 6, 2013

1 check passed

Details default The Travis build passed
@twpayne twpayne deleted the twpayne:controls-api branch Mar 6, 2013
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.