Make ol.control.Control extends ol.Object #800
Conversation
|
Nice. I like what you're doing with this :) |
| * @inheritDoc | ||
| */ | ||
| ol.control.ScaleLine.prototype.handleMapPostrender = function(mapEvent) { | ||
| this.updateElement_(mapEvent.frameState); | ||
| this.frameState_ = mapEvent.frameState; |
There was a problem hiding this comment.
I'm a bit unsure with storing the frameState in the instance, and reusing it later when handleUnitsChanged_ is called.
There was a problem hiding this comment.
Because we have to call this.updateElement_() when the units changed but we don't have a frameState at this moment.
There was a problem hiding this comment.
Good points both. I would be tempted just to store the values from frameState that we need in this.updateElement_().
There was a problem hiding this comment.
I know. I just don't like the idea of storing the frameState in the instance. A frameState is a volatile object that has a meaning within its frame only.
There was a problem hiding this comment.
Good points both. I would be tempted just to store the values from frameState that we need in this.updateElement_().
Which means center, resolution and projection. We can maybe store the entire view2DState object. I'm probably a bit pedantic here, but storing the "frame state" for use outside a "frame" bothers me :)
There was a problem hiding this comment.
Good point; storing the view2DState (or a subset) is perfectly fine (and much more logical).
There was a problem hiding this comment.
+1 on view2DState. Good idea.
|
ready for review, thanks. |
| }, false); | ||
| var projection = new ol.dom.Input(document.getElementById('projection')); | ||
| projection.on('change:value', function() { | ||
| control.setProjection(ol.proj.get(this.getValue())); |
There was a problem hiding this comment.
Using projection rather than this here would be more explicit.
There was a problem hiding this comment.
good point, fixed (also for precision)
|
I also like this. |
| goog.require('ol.layer.TileLayer'); | ||
| goog.require('ol.source.OSM'); | ||
|
|
||
|
|
||
| var control = new ol.control.ScaleLine(); |
There was a problem hiding this comment.
I'd rename control to scaleLineControl to make the following code a little clearer.
|
A couple of minor comments, but otherwise this looks VGTM. |
Make ol.control.Control extends ol.Object
Use GPU where available; animated zooming. r=@elemoine
With the map's applyTransform method, the control does not need and have its own applyTransform and center management any more.
This still doesn't address the broken ZoomBox test (see openlayers#800), it fails in the same way with or without this change.
As suggested by @twpayne in #688 (comment), make the controls extends
ol.Objectinstead ofgoog.Disposable.