[not ready] create fullscreen example and control #3977

colleenmcginnis · 2017-01-14T00:49:56Z

Creates a fullscreen control and an example using the fullscreen control to address #1824.

Hoping @mollymerp and @lucaswoj can help with some things that still need to be resolved:

Do the control and the example need to be in two different PRs (master and mb-pages)?
The current control works in example, but not in the debug page. @mollymerp looked at this earlier - do you remember what the issue was?
In addition to changing the icon when the button is clicked, it also needs to change when the ESC key is pressed. Not sure what the best way is to do that as a part of the control.
There are several errors in the js/ui/control/fullscreen.js file that I'm not sure how to interpret or resolve.

mollymerp · 2017-01-14T01:47:51Z

awesome work @colleenmcginnis !!

no need to separate the master and mb-pages PRs bc we wouldn't want the example to show up before the control is released. master is merged into mb-pages on every release
I do remember the thing with the debug page. I'll take a look into it on Monday
I think you'll need to add a case in https://github.com/mapbox/mapbox-gl-js/blob/master/js/ui/handler/keyboard.js to handle the esc key functionality
I'll go through and see if I can help w the errors you're seeing.

andrewharvey · 2017-01-14T02:00:46Z

js/ui/control/fullscreen.js

+        var container = this._container = DOM.create('div', className + '-group', map.getContainer());
+        var button = this._fullscreenButton = DOM.create('button', (className + '-icon ' + className + '-fullscreen'), this._container);
+        button.id = 'fullscreen-button';
+        this._fullscreenButton.type = 'button';


@colleenmcginnis Could you include an aria-label attribute on the button with value of say "Fullscreen" for accessibility?

Similar to how it's done for the navigation buttons https://github.com/mapbox/mapbox-gl-js/blob/master/js/ui/control/navigation_control.js#L95

mollymerp · 2017-01-14T01:52:42Z

js/ui/control/fullscreen.js

+    util.setOptions(this, options);
+}
+
+Fullscreen.prototype = util.inherit(Control, {


We deprecated the Control class, and now controls don't need to inherit from anything, but they do need to follow the implementation of IControl (i.e. need onAdd, onRemove functions. You can also add a getDefaultPosition function to replace the options object passed to the Control).

mollymerp · 2017-01-14T02:15:41Z

js/ui/control/fullscreen.js

+        if (window.innerHeight === screen.height) {
+            button.classList.remove(className + '-shrink');
+            button.classList.add(className + '-fullscreen');
+            if (document.exitFullscreen) {


I think you may need to require the window util and use window.document.exitFullscreen etc to get past the linting errors.

jfirebaugh · 2017-01-17T02:15:21Z

dist/mapbox-gl.css

@@ -267,3 +274,11 @@
        display:none;
    }
 }
+.pseudo-fullscreen {


This doesn't seem to be used (and we shouldn't need to support "pseudo-fullscreen" mode, as all browsers supported by gl-js should support the HTML5 fullscreen API).

jfirebaugh · 2017-01-17T02:16:31Z

js/ui/control/fullscreen.js

+
+    _onClickFullscreen: function() {
+        var mapContainer = map.getContainer();
+        var button = document.getElementById('fullscreen-button');


Use this._fullscreenButton rather than looking up the element by ID.

jfirebaugh · 2017-01-17T02:17:12Z

js/ui/control/fullscreen.js

+        var className = 'mapboxgl-ctrl';
+        var container = this._container = DOM.create('div', className + '-group', map.getContainer());
+        var button = this._fullscreenButton = DOM.create('button', (className + '-icon ' + className + '-fullscreen'), this._container);
+        button.id = 'fullscreen-button';


Avoid assigning an ID -- IDs are required to be unique, and there may be more than one map with fullscreen control on a page.

mollymerp

Looking good @colleenmcginnis -- just a couple small changes. In order to add the documentation you write to the website, you'll also need to add it in the documentation.yml file with the other controls.

mollymerp · 2017-01-24T21:11:47Z

debug/index.html

+map.addControl(new mapboxgl.NavigationControl());
+map.addControl(new mapboxgl.GeolocateControl());
+map.addControl(new mapboxgl.ScaleControl());
+map.addControl(new mapboxgl.FullscreenControl());


can you revert the changes on this file so it matches the debug/index.html file on master? you can add a new debug page for controls instead if you want to.

mollymerp · 2017-01-24T21:12:54Z

dist/mapbox-gl.css

@@ -85,7 +85,14 @@
 .mapboxgl-ctrl-icon.mapboxgl-ctrl-geolocate.watching  {
    background-image: url("data:image/svg+xml;charset=utf-8,%3Csvg%20viewBox%3D%270%200%2020%2020%27%20xmlns%3D%27http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%27%3E%0D%0A%20%20%3Cpath%20style%3D%27fill%3A%2300f%3B%27%20d%3D%27M10%204C9%204%209%205%209%205L9%205.1A5%205%200%200%200%205.1%209L5%209C5%209%204%209%204%2010%204%2011%205%2011%205%2011L5.1%2011A5%205%200%200%200%209%2014.9L9%2015C9%2015%209%2016%2010%2016%2011%2016%2011%2015%2011%2015L11%2014.9A5%205%200%200%200%2014.9%2011L15%2011C15%2011%2016%2011%2016%2010%2016%209%2015%209%2015%209L14.9%209A5%205%200%200%200%2011%205.1L11%205C11%205%2011%204%2010%204zM10%206.5A3.5%203.5%200%200%201%2013.5%2010%203.5%203.5%200%200%201%2010%2013.5%203.5%203.5%200%200%201%206.5%2010%203.5%203.5%200%200%201%2010%206.5zM10%208.3A1.8%201.8%200%200%200%208.3%2010%201.8%201.8%200%200%200%2010%2011.8%201.8%201.8%200%200%200%2011.8%2010%201.8%201.8%200%200%200%2010%208.3z%27%20%2F%3E%0D%0A%3C%2Fsvg%3E");
 }
-
+.mapboxgl-ctrl-icon.mapboxgl-ctrl-fullscreen  {
+    padding: 5px;


I don't think you need the padding here because its included here

mollymerp · 2017-01-24T21:13:07Z

dist/mapbox-gl.css

+    background-image: url("data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0iVVRGLTgiIHN0YW5kYWxvbmU9Im5vIj8+CjwhLS0gR2VuZXJhdG9yOiBBZG9iZSBJbGx1c3RyYXRvciAxOS4wLjEsIFNWRyBFeHBvcnQgUGx1Zy1JbiAuIFNWRyBWZXJzaW9uOiA2LjAwIEJ1aWxkIDApICAtLT4KCjxzdmcKICAgeG1sbnM6ZGM9Imh0dHA6Ly9wdXJsLm9yZy9kYy9lbGVtZW50cy8xLjEvIgogICB4bWxuczpjYz0iaHR0cDovL2NyZWF0aXZlY29tbW9ucy5vcmcvbnMjIgogICB4bWxuczpyZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1ucyMiCiAgIHhtbG5zOnN2Zz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciCiAgIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIKICAgeG1sbnM6c29kaXBvZGk9Imh0dHA6Ly9zb2RpcG9kaS5zb3VyY2Vmb3JnZS5uZXQvRFREL3NvZGlwb2RpLTAuZHRkIgogICB4bWxuczppbmtzY2FwZT0iaHR0cDovL3d3dy5pbmtzY2FwZS5vcmcvbmFtZXNwYWNlcy9pbmtzY2FwZSIKICAgdmVyc2lvbj0iMS4xIgogICBpZD0iTGF5ZXJfMSIKICAgeD0iMHB4IgogICB5PSIwcHgiCiAgIHZpZXdCb3g9IjAgMCAyMCAyMCIKICAgc3R5bGU9ImVuYWJsZS1iYWNrZ3JvdW5kOm5ldyAwIDAgMjAgMjA7IgogICB4bWw6c3BhY2U9InByZXNlcnZlIgogICBpbmtzY2FwZTp2ZXJzaW9uPSIwLjkxIHIxMzcyNSIKICAgc29kaXBvZGk6ZG9jbmFtZT0iZnVsbHNjcmVlbi5zdmciPjxtZXRhZGF0YQogICAgIGlkPSJtZXRhZGF0YTQxODUiPjxyZGY6UkRGPjxjYzpXb3JrCiAgICAgICAgIHJkZjphYm91dD0iIj48ZGM6Zm9ybWF0PmltYWdlL3N2Zyt4bWw8L2RjOmZvcm1hdD48ZGM6dHlwZQogICAgICAgICAgIHJkZjpyZXNvdXJjZT0iaHR0cDovL3B1cmwub3JnL2RjL2RjbWl0eXBlL1N0aWxsSW1hZ2UiIC8+PGRjOnRpdGxlPjwvZGM6dGl0bGU+PC9jYzpXb3JrPjwvcmRmOlJERj48L21ldGFkYXRhPjxkZWZzCiAgICAgaWQ9ImRlZnM0MTgzIiAvPjxzb2RpcG9kaTpuYW1lZHZpZXcKICAgICBwYWdlY29sb3I9IiNmZmZmZmYiCiAgICAgYm9yZGVyY29sb3I9IiM2NjY2NjYiCiAgICAgYm9yZGVyb3BhY2l0eT0iMSIKICAgICBvYmplY3R0b2xlcmFuY2U9IjEwIgogICAgIGdyaWR0b2xlcmFuY2U9IjEwIgogICAgIGd1aWRldG9sZXJhbmNlPSIxMCIKICAgICBpbmtzY2FwZTpwYWdlb3BhY2l0eT0iMCIKICAgICBpbmtzY2FwZTpwYWdlc2hhZG93PSIyIgogICAgIGlua3NjYXBlOndpbmRvdy13aWR0aD0iMTQ3MSIKICAgICBpbmtzY2FwZTp3aW5kb3ctaGVpZ2h0PSI2OTUiCiAgICAgaWQ9Im5hbWVkdmlldzQxODEiCiAgICAgc2hvd2dyaWQ9ImZhbHNlIgogICAgIGlua3NjYXBlOnpvb209IjExLjMxMzcwOCIKICAgICBpbmtzY2FwZTpjeD0iMTQuNjk4MjgiCiAgICAgaW5rc2NhcGU6Y3k9IjEwLjUyNjY4OSIKICAgICBpbmtzY2FwZTp3aW5kb3cteD0iNjk3IgogICAgIGlua3NjYXBlOndpbmRvdy15PSIyOTgiCiAgICAgaW5rc2NhcGU6d2luZG93LW1heGltaXplZD0iMCIKICAgICBpbmtzY2FwZTpjdXJyZW50LWxheWVyPSJMYXllcl8xIgogICAgIGlua3NjYXBlOnNuYXAtYmJveD0idHJ1ZSIKICAgICBpbmtzY2FwZTpiYm94LXBhdGhzPSJ0cnVlIgogICAgIGlua3NjYXBlOm9iamVjdC1wYXRocz0idHJ1ZSIKICAgICBpbmtzY2FwZTpiYm94LW5vZGVzPSJ0cnVlIgogICAgIGlua3NjYXBlOm9iamVjdC1ub2Rlcz0idHJ1ZSI+PGlua3NjYXBlOmdyaWQKICAgICAgIHR5cGU9Inh5Z3JpZCIKICAgICAgIGlkPSJncmlkNjA3NiIgLz48L3NvZGlwb2RpOm5hbWVkdmlldz48cGF0aAogICAgIGQ9Ik0gNSA0IEMgNC41IDQgNCA0LjUgNCA1IEwgNCA2IEwgNCA5IEwgNC41IDkgTCA1Ljc3NzM0MzggNy4yOTY4NzUgQyA2Ljc3NzEzMTkgOC4wNjAyMTMxIDcuODM1NzY1IDguOTU2NTcyOCA4Ljg5MDYyNSAxMCBDIDcuODI1NzEyMSAxMS4wNjMzIDYuNzc2MTc5MSAxMS45NTE2NzUgNS43ODEyNSAxMi43MDcwMzEgTCA0LjUgMTEgTCA0IDExIEwgNCAxNSBDIDQgMTUuNSA0LjUgMTYgNSAxNiBMIDkgMTYgTCA5IDE1LjUgTCA3LjI3MzQzNzUgMTQuMjA1MDc4IEMgOC4wNDI4OTMxIDEzLjE4Nzg4NiA4LjkzOTU0NDEgMTIuMTMzNDgxIDkuOTYwOTM3NSAxMS4wNjgzNTkgQyAxMS4wNDIzNzEgMTIuMTQ2OTkgMTEuOTQyMDkzIDEzLjIxMTIgMTIuNzA3MDMxIDE0LjIxODc1IEwgMTEgMTUuNSBMIDExIDE2IEwgMTQgMTYgTCAxNSAxNiBDIDE1LjUgMTYgMTYgMTUuNSAxNiAxNSBMIDE2IDE0IEwgMTYgMTEgTCAxNS41IDExIEwgMTQuMjA1MDc4IDEyLjcyNjU2MiBDIDEzLjE3Nzk4NSAxMS45NDk2MTcgMTIuMTEyNzE4IDExLjA0MzU3NyAxMS4wMzcxMDkgMTAuMDA5NzY2IEMgMTIuMTUxODU2IDguOTgxMDYxIDEzLjIyNDM0NSA4LjA3OTg2MjQgMTQuMjI4NTE2IDcuMzA0Njg3NSBMIDE1LjUgOSBMIDE2IDkgTCAxNiA1IEMgMTYgNC41IDE1LjUgNCAxNSA0IEwgMTEgNCBMIDExIDQuNSBMIDEyLjcwMzEyNSA1Ljc3NzM0MzggQyAxMS45MzI2NDcgNi43ODY0ODM0IDExLjAyNjY5MyA3Ljg1NTQ3MTIgOS45NzA3MDMxIDguOTE5OTIxOSBDIDguOTU4NDczOSA3LjgyMDQ5NDMgOC4wNjk4NzY3IDYuNzYyNzE4OCA3LjMwNDY4NzUgNS43NzE0ODQ0IEwgOSA0LjUgTCA5IDQgTCA2IDQgTCA1IDQgeiAiCiAgICAgaWQ9InBhdGg0MTY5IiAvPjwvc3ZnPg==");
+}
+.mapboxgl-ctrl-icon.mapboxgl-ctrl-shrink  {
+    padding: 5px;


same as L89

mollymerp · 2017-01-24T21:26:13Z

js/ui/control/fullscreen_control.js

+const util = require('../../util/util');
+const window = require('../../util/window');
+
+class FullscreenControl {


can you add documentation markup here? here is an example from the NavigationControl

/** * A `NavigationControl` control contains zoom buttons and a compass. * * @implements {IControl} * @example * var nav = new mapboxgl.NavigationControl(); * map.addControl(nav, 'top-left'); * @see [Display map navigation controls](https://www.mapbox.com/mapbox-gl-js/example/navigation/) * @see [Add a third party vector tile source](https://www.mapbox.com/mapbox-gl-js/example/third-party/) */

mollymerp · 2017-01-24T21:32:59Z

package.json

@@ -117,7 +117,7 @@
    "build-docs": "documentation build --github --format html --config documentation.yml --theme ./docs/_theme --output docs/api/",
    "build": "npm run build-docs # invoked by publisher when publishing docs on the mb-pages branch",
    "start-docs": "npm run build-min && npm run build-docs && jekyll serve --watch",
-    "lint": "eslint --ignore-path .gitignore js test bench docs/_posts/examples/*.html debug/*.html",
+    "lint": "eslint --fix --ignore-path .gitignore js test bench docs/_posts/examples/*.html debug/*.html",


please revert this as well 😉

I recently learned that you can do this with npm:

npm run lint -- --fix

.. and it'll pass the --fix flag down to the eslint command 😄

mollymerp · 2017-01-30T18:05:16Z

js/ui/control/fullscreen_control.js

+        util.bindAll([
+            '_isFullscreen',
+            '_onClickFullscreen',
+            '_onKeyDown',


You can get rid of _onKeyDown here -- I think you deleted that function from a previous iteration.

Also I don't think you need to bind _isFullscreen bc its not used as an event listener.

jfirebaugh · 2017-02-04T00:48:02Z

js/ui/control/fullscreen_control.js

+        button.setAttribute("aria-label", "Toggle fullscreen");
+        this._fullscreenButton.addEventListener('click', this._onClickFullscreen);
+        this._mapContainer = map.getContainer();
+        window.document.addEventListener('webkitfullscreenchange', this._changeIcon);


webkitfullscreenchange is specific to WebKit -- you'll need to do feature detection to determine the appropriate event name for other browsers. Here's how the Leaflet equivalent does it.

jfirebaugh · 2017-02-04T00:48:25Z

js/ui/control/fullscreen_control.js

+    }
+
+    onRemove() {
+        this._container.parentNode.removeChild(this._container);


onRemove should unbind the fullscreenchange event handler, using window.document.removeEventListener.

jfirebaugh · 2017-02-04T00:50:04Z

js/ui/control/fullscreen_control.js

+        return this._fullscreen;
+    }
+
+    _changeIcon() {


_changeIcon, as the event handler bound to fullscreenchange event, needs to check if the event corresponds to the appropriate page element. There might be more than one map on the page, or other elements that go fullscreen. Here's the equivalent code in the Leaflet plugin.

jfirebaugh · 2017-02-04T00:50:48Z

js/ui/control/fullscreen_control.js

+    }
+
+    _onClickFullscreen() {
+        if (this._isFullscreen()) {


There doesn't seem to be a case in this function for IE11 (msRequestFullscreen / msExitFullscreen).

colleenmcginnis · 2017-02-22T18:18:48Z

@jfirebaugh - @mollymerp and I worked on addressing your comments the other day. Can you take another look when you get the chance? Thanks!

jfirebaugh

Nice work! Just one last thing I noticed.

jfirebaugh · 2017-02-22T18:24:18Z

src/ui/control/fullscreen_control.js

+    onRemove() {
+        this._container.parentNode.removeChild(this._container);
+        this._map = null;
+        window.document.removeEventListener(this._fullscreenchange);


A second argument is needed here -- removeEventListener(this._fullscreenchange, this._changeIcon).

colleenmcginnis added the needs revision label Jan 14, 2017

colleenmcginnis force-pushed the ex-fullscreen branch from 4c5aff4 to f67bfa5 Compare January 14, 2017 01:21

andrewharvey reviewed Jan 14, 2017

View reviewed changes

mollymerp reviewed Jan 14, 2017

View reviewed changes

jfirebaugh reviewed Jan 17, 2017

View reviewed changes

colleenmcginnis force-pushed the ex-fullscreen branch from f67bfa5 to 9d28a22 Compare January 24, 2017 02:59

mollymerp suggested changes Jan 24, 2017

View reviewed changes

mollymerp reviewed Jan 24, 2017

View reviewed changes

mollymerp reviewed Jan 30, 2017

View reviewed changes

lucaswoj added under development and removed needs revision labels Jan 31, 2017

mollymerp approved these changes Feb 3, 2017

View reviewed changes

jfirebaugh suggested changes Feb 4, 2017

View reviewed changes

colleenmcginnis added 11 commits February 20, 2017 15:28

create fullscreen example and control

31c4468

add fullscreen control

610970c

toggle fullscreen

4205505

clean up code

f242685

set height and width

367662f

change icon on esc

6fefdbe

add fullscreen to debug

2e79e81

address merp comments

b205de0

remove _onKeyDown and _isFullscreen

36c17db

address johns comments

b64e67d

move file to src folder and add case for IE11

945b047

colleenmcginnis force-pushed the ex-fullscreen branch from d05e180 to 945b047 Compare February 21, 2017 00:02

add newline to debug page

8d785fc

jfirebaugh approved these changes Feb 22, 2017

View reviewed changes

add second argument

1b0fecc

colleenmcginnis merged commit 78fc9c4 into master Feb 25, 2017

colleenmcginnis deleted the ex-fullscreen branch February 25, 2017 02:02

mollymerp mentioned this pull request Feb 25, 2017

Create an example for fullscreen mode #1824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[not ready] create fullscreen example and control #3977

[not ready] create fullscreen example and control #3977

colleenmcginnis commented Jan 14, 2017 •

edited

Loading

mollymerp commented Jan 14, 2017

andrewharvey Jan 14, 2017

mollymerp Jan 14, 2017

mollymerp Jan 14, 2017

jfirebaugh Jan 17, 2017

jfirebaugh Jan 17, 2017

jfirebaugh Jan 17, 2017

mollymerp left a comment

mollymerp Jan 24, 2017 •

edited

Loading

mollymerp Jan 24, 2017

mollymerp Jan 24, 2017

mollymerp Jan 24, 2017

mollymerp Jan 24, 2017

lucaswoj Jan 25, 2017

mollymerp Jan 30, 2017

mollymerp Jan 30, 2017

jfirebaugh Feb 4, 2017

jfirebaugh Feb 4, 2017

jfirebaugh Feb 4, 2017

jfirebaugh Feb 4, 2017

colleenmcginnis commented Feb 22, 2017

jfirebaugh left a comment

jfirebaugh Feb 22, 2017

[not ready] create fullscreen example and control #3977

[not ready] create fullscreen example and control #3977

Conversation

colleenmcginnis commented Jan 14, 2017 • edited Loading

mollymerp commented Jan 14, 2017

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

mollymerp left a comment

Choose a reason for hiding this comment

mollymerp Jan 24, 2017 • edited Loading

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

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

colleenmcginnis commented Feb 22, 2017

jfirebaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colleenmcginnis commented Jan 14, 2017 •

edited

Loading

mollymerp Jan 24, 2017 •

edited

Loading