Add zoom to extent control #824

sbrunner · 2013-06-26T08:35:36Z

I create an example page named navigation-controls to have a common example for 'Zoom to extent', 'Zoom to position' and 'Pan buttons'.

----------------------------------------- Solved -----------------------------------------
I try ./build.py check and ./build.py integration-test, but I have an exception:

Traceback (most recent call last):
  File "./build.py", line 759, in <module>
    main()
  File "/home/sbrunner/workspace/ol3/pake.py", line 339, in main
    target.build(dry_run=options.dry_run)
  File "/home/sbrunner/workspace/ol3/pake.py", line 92, in build
    timestamp = max(timestamp, target.build(dry_run=dry_run))
  File "/home/sbrunner/workspace/ol3/pake.py", line 92, in build
    timestamp = max(timestamp, target.build(dry_run=dry_run))
  File "/home/sbrunner/workspace/ol3/pake.py", line 107, in build
    self.action(self)
  File "./build.py", line 358, in build_lint_src_timestamp
    t.newer(t.dependencies))
  File "/home/sbrunner/workspace/ol3/pake.py", line 207, in run
    subprocess.check_call(args, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 506, in check_call
    retcode = call(*popenargs, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 493, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 679, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1249, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

But I have the same exception on master...

elemoine · 2013-06-26T08:40:19Z

Do you have the closure_linter package installed and gjslint available on your PATH? See https://developers.google.com/closure/utilities/docs/linter_howto.

sbrunner · 2013-06-26T08:45:48Z

Thanks, than I will fix the errors :-)

sbrunner · 2013-06-26T09:27:51Z

Tests pass than it's ready to review :-)

twpayne · 2013-06-26T09:48:06Z

examples/navigation-controls.html

+      <div class="row-fluid">
+
+        <div class="span4">
+          <h4 id="title">Scale line example</h4>


This should be "Navigation controls example".

twpayne · 2013-06-26T09:51:04Z

When I run the example, I don't actually see the control on the map. Where is it supposed to be?

sbrunner · 2013-06-26T10:14:36Z

Strange, in my browser it's working ...

Do you have an error ?
Or we can simply see it together tomorrow.

sbrunner · 2013-06-26T10:15:42Z

Oups I just see that it's not working on Chromium ... I will fix it :-)

sbrunner · 2013-06-26T10:20:31Z

I just drop the cache to make it working ...

sbrunner · 2013-06-26T10:23:21Z

I update the example page (should be a fixup before merging !)

fredj · 2013-06-26T10:30:50Z

More generally; do we want/need this control?
Besides the default extent from the projection, a zoom to extent button could be simply:

diff --git a/examples/simple.html b/examples/simple.html
index 5891875..6611499 100644
--- a/examples/simple.html
+++ b/examples/simple.html
@@ -40,6 +40,7 @@
           <p id="shortdesc">Example of a simple map.</p>
           <div id="docs">
             <p>See the <a href="simple.js" target="_blank">simple.js source</a> to see how this is done.</p>
+           <button id="max-extent" class="btn">zoom to max extent</button>
           </div>
           <div id="tags">simple, openstreetmap</div>
         </div>
diff --git a/examples/simple.js b/examples/simple.js
index b1335ef..920d00d 100644
--- a/examples/simple.js
+++ b/examples/simple.js
@@ -18,3 +18,7 @@ var map = new ol.Map({
     zoom: 2
   })
 });
+
+document.getElementById('max-extent').addEventListener('click', function() {
+  map.getView().fitExtent([813079, 848966, 5929220, 5936863], map.getSize());
+});

twpayne · 2013-06-26T11:07:31Z

@sbrunner, thanks it was indeed a stale stylesheet in my browser cache. I've checked the example Chrome, Firefox and Safari on Mac OS X and all work as expected.

@fredj, even those the same effect can be achieved in a few lines of code, I think it's worth including this control in the core of ol3. It's a few lines of code less for users to write, and is properly integrated into ol3's control architecture. With the Closure Compiler/build system, if you don't use it you don't pay for it, so it doesn't have any negative effect on the build size.

twpayne · 2013-06-26T11:08:18Z

LGTM

elemoine · 2013-06-26T12:05:16Z

Having a look right now.

elemoine · 2013-06-26T12:07:11Z

src/ol/control/zoomtoextentcontrol.exports

@@ -0,0 +1,2 @@
+@exportClass ol.control.ZoomToExtent ol.control.ZoomToExtentOptions
+@exportProperty ol.control.ZoomToExtent.prototype.setMap


controls.exports already exports setMap, so it shouldn't be needed here.

I haven't tested, but I think that, due to the way exports work, it is necessary to re-export overridden methods.

My understand is that exporting really creates an alias:

Base.prototype.mangledMethod = function () { /* implementation */ }; // export then does Base.prototype.exportedMethod = Base.prototype.mangledMethod

In this case, calling exportedMethod on an instance of a child class will actually give you the base class's implementation since it's the only method of that name in the instance's prototype chain.

Corrections welcome.

I think you're absolutely correct on that.

elemoine · 2013-06-26T12:34:42Z

@fredj, even those the same effect can be achieved in a few lines of code, I think it's worth including this control in the core of ol3. It's a few lines of code less for users to write, and is properly integrated into ol3's control architecture. With the Closure Compiler/build system, if you don't use it you don't pay for it, so it doesn't have any negative effect on the build size.

I agree. And styling <button> isn't easy. Pre-styled button controls make sense to me.

sbrunner · 2013-06-26T13:03:31Z

@elemoine

This commit message is not very clear. We don't even know what example you're talking about. I would suggest to merge that commit into the previous one. Or better, you could have a commit for the example specifically.

Yes, I already plan to merge them

If we add a ZoomToExtent control implementing a ZoomToExtent control in the custom-controls example does not make too much sense. We could have a ResetRotation control in custom-controls in place of ZoomToExtent.

Yes, do you have a quick input of how to do it ?

I don't find the navigation-controls explicit enough. That example shows how to use the ol.control.ZoomToExtent control so I'd rename it zoom-to-extent.

It's because I plan to create only one example for 'Zoom to extent', 'Zoom to position' and 'Pan buttons'. If it's not a good idea I can do one example per control ...

twpayne · 2013-06-26T13:13:14Z

To reset the rotation, call

map.getView().getView2D().setRotation(0);

As @elemoine suggests, it makes sense to replace the custom controls example with a different control, but I think it's OK to do this in a separate pull request.

Having a single example showing several navigation controls makes sense to me.

elemoine · 2013-06-26T13:18:07Z

It's because I plan to create only one example for 'Zoom to extent', 'Zoom to position' and 'Pan buttons'. If it's not a good idea I can do one example per control ...

Ok. Didn't know that was your intent.

elemoine · 2013-06-26T13:18:26Z

As @elemoine suggests, it makes sense to replace the custom controls example with a different control, but I think it's OK to do this in a separate pull request.

Yes.

sbrunner · 2013-06-26T13:21:27Z

Thanks, I will modify the custom-controls example, is it OK if I steal have one commit for the control and the example ?

elemoine · 2013-06-26T13:51:55Z

I'd personally do two commits: one for the control and one for the example. Easy to do:

$ git reset HEAD^
$ git add <all files related to the control>
$ git commit -m 'Add ol.control.ZoomToExtent'
$ git add examples/navigation-controls.html examples/navigation-controls.js
$ git commit -m 'Add navigation-controls example

sbrunner · 2013-06-26T16:36:05Z

I think that I take care on all your comment :-)

elemoine · 2013-06-26T16:43:47Z

src/ol/control/zoomtoextentcontrol.js

+    'title': goog.isDef(options.title) ? options.title :
+        'Zoom to initial extent'
+  });
+  goog.dom.appendChild(element, button);


The button is still a div instead of an anchor. You didn't want to change that?

I don't understand why we create an anchor ant transom it (especially with css) to a div, is it needed for accessibility ?

We don't transform it to a div. Please look at the zoom control.

I see:

.ol-zoom a { display: block; text-decoration: none; color: white; }

and

// prevent #zoomIn anchor from getting appended to the url browserEvent.preventDefault();

Every (as I know) specific thing of an anchor are removed => div (from me) !

In case it's not clear: by anchor I mean a <a> element here. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a.

Phone call => it's for accessibility :-)

elemoine · 2013-06-26T16:44:25Z

src/ol/control/zoomtoextentcontrol.js

+  if (goog.isDef(options.html)) {
+    var inner = goog.dom.htmlToDocumentFragment(options.html);
+    goog.dom.appendChild(button, inner);
+  }


The zoom control has no html option. The content is set with CSS.

sbrunner · 2013-06-27T09:13:17Z

Shouldn't we create a generic CSS class like .ol-buttons-group for the control buttons ?
with definition like this:

.ol-buttons-group {
  position: absolute;
  background: rgba(255,255,255,0.4);
  border-radius: 4px;
  padding: 2px;
}
@media print {
  .ol-buttons-group {
    display: none;
  }
}
.ol-buttons-group div { /* or a ? */
  cursor: pointer;
  margin: 1px;
  height: 22px;
  width: 22px;
  background-color: rgba(0, 60, 136, 0.5);
  border-radius: 2px;
}
.ol-buttons-group div:first {
  border-top-left-radius: 2px;
  border-top-right-radius: 2px;
}
.ol-buttons-group div:last {
  border-bottom-left-radius: 2px;
  border-bottom-right-radius: 2px;
}
.ol-buttons-group div:hover {
  background-color: rgba(0, 60, 136, 0.7);
}

fredj · 2013-06-27T11:54:52Z

@sbrunner see: https://github.com/fredj/ol3/compare/control_button

sbrunner · 2013-06-27T12:44:59Z

I remove move my last commit in the https://github.com/sbrunner/ol3/compare/button-class branch, than for me this pull request is ready to merge :-)

elemoine · 2013-06-27T12:50:46Z

src/ol/control/zoomtoextentcontrol.js

+  var button = goog.dom.createDom(goog.dom.TagName.A, {
+    'href': '#zoomExtent',
+    'title': goog.isDef(options.title) ? options.title :
+        'Zoom to initial extent'


"Zoom to initial extent" is not correct. There's no notion of "initial extent". I'd remove "title" entirely for now.

sbrunner · 2013-06-27T13:14:56Z

Thanks @elemoine, removed

sbrunner · 2013-06-28T15:25:53Z

@fredj branch updated https://github.com/sbrunner/ol3/compare/control_button (depends on this pull request)

elemoine · 2013-07-11T07:48:30Z

It looks good. I just added a few more comments, which I will address myself (@sbrunner is on vacation).

sbrunner · 2013-07-16T09:17:12Z

Replaced by #859

twpayne reviewed Jun 26, 2013
View reviewed changes

elemoine reviewed Jun 26, 2013
View reviewed changes

elemoine reviewed Jun 27, 2013
View reviewed changes

sbrunner added 3 commits June 28, 2013 10:12

Add zoom to extent control

97f5ebc

Add navigation controls example

a1997b4

Modify the custom control to rotate to north instance of zoom to extent

0f78ea9

elemoine mentioned this pull request Jul 11, 2013

Add a ZoomToExtent control #859

Merged

sbrunner closed this Jul 16, 2013

		@@ -0,0 +1,2 @@
		@exportClass ol.control.ZoomToExtent ol.control.ZoomToExtentOptions
		@exportProperty ol.control.ZoomToExtent.prototype.setMap

Add zoom to extent control #824

Add zoom to extent control #824

Conversation

sbrunner commented Jun 26, 2013

elemoine commented Jun 26, 2013

sbrunner commented Jun 26, 2013

sbrunner commented Jun 26, 2013

Choose a reason for hiding this comment

twpayne commented Jun 26, 2013

sbrunner commented Jun 26, 2013

sbrunner commented Jun 26, 2013

sbrunner commented Jun 26, 2013

sbrunner commented Jun 26, 2013

fredj commented Jun 26, 2013

twpayne commented Jun 26, 2013

twpayne commented Jun 26, 2013

elemoine commented Jun 26, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elemoine commented Jun 26, 2013

sbrunner commented Jun 26, 2013

twpayne commented Jun 26, 2013

elemoine commented Jun 26, 2013

elemoine commented Jun 26, 2013

sbrunner commented Jun 26, 2013

elemoine commented Jun 26, 2013

sbrunner commented Jun 26, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbrunner commented Jun 27, 2013

fredj commented Jun 27, 2013

sbrunner commented Jun 27, 2013

Choose a reason for hiding this comment

sbrunner commented Jun 27, 2013

sbrunner commented Jun 28, 2013

elemoine commented Jul 11, 2013

sbrunner commented Jul 16, 2013