add Mapbox wordmark as LogoControl #3933

mollymerp · 2017-01-10T01:52:09Z

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores
manually test the debug page

mourner · 2017-01-10T14:39:12Z

js/ui/control/logo_control.js

+
+class LogoControl {
+    constructor() {
+    }


you can omit empty constructors

mourner · 2017-01-10T14:39:56Z

js/ui/control/logo_control.js

+    }
+
+    onAdd(map) {
+        this._map = map;


Do we need to save the map reference if it's not used by the control?

I guess we don't! I was following the IControl docs, but you're right it isn't necessary in this case.

mourner · 2017-01-10T14:41:29Z

js/ui/map.js

@@ -200,6 +203,7 @@ class Map extends Camera {
        if (options.style) this.setStyle(options.style);

        if (options.attributionControl) this.addControl(new AttributionControl());
+        if (options.logoControl) this.addControl(new LogoControl(), 'bottom-left');


I think it's better to code the default position in the control code by adding a getDefaultPosition method.

jfirebaugh

The equivalent control in Mapbox.js is shown or hidden automatically based on a status flag returned by the Mapbox API. We'll likely need to implement a similar facility in the Styles API.

mollymerp · 2017-01-10T23:53:29Z

@jfirebaugh do you mean we should punt on this until we implement something at the TileJSON level?

I'm looking in to what implementing a flag like that looks like, but some enterprise customers will still need to be able to remove the logo even if they're using our vector tiles depending on the terms of their Enterprise agreement.

mollymerp · 2017-01-11T01:19:50Z

Per chat in slack and convos w @sean-heff @ErinQuinn I think we should add the logo as on by default, and actually not document how to remove it. This would be the same behavior that we have currently implemented in the iOS SDK (per @1ec5) and be the most effective way to prevent users from unwillingly violating our TOS.

we have lines of communication open with all users that have approved white labeling (customer success) and so if they want info on how to remove the logo, we can provide it.

mourner · 2017-01-11T09:06:39Z

@mollymerp it seems to be implemented at the TileJSON level (with mapbox_logo property) — you would need to listen to map source.load event and check whether e.source.mapbox_logo is true.

mollymerp · 2017-01-11T18:02:31Z

~@mourner I think the mapbox_logo flag is only enabled for mapbox.js tilesets because I don't see it coming through in the tileJSON in mapbox-gl-js.~~(👈 that was incorrect) That flag automatically disables the logo for any enterprise accounts, which is not ideal because only a fraction of enterprise accounts actually have the rights to remove the logo from their maps.

Both the Android and iOS SDKs enable the Mapbox logo automatically for all users and I think it makes most sense to follow this pattern.

andrewharvey · 2017-01-12T09:39:23Z

@mollymerp Not required but might be a good idea to add aria-label="Mapbox logo" to that element for better accessibility support.

mollymerp · 2017-01-12T18:00:46Z

@andrewharvey thank you so much for catching that! I'll have to get better about making a habit of including these.

andrewharvey · 2017-01-17T23:41:18Z

While technically not a breaking change, it will act as a breaking change for those who've followed the current advice and manually added the logo (as they'll end up with two overlapping logos, potentially slightly offset from each other). So a notice about this in the CHANGELOG when released will be welcome.

lucaswoj · 2017-01-18T21:17:21Z

We are going to collaborate with the teams that maintain Mapbox's APIs to implement a mapbox_logo-like property before moving forward with this PR.

mollymerp · 2017-01-18T22:56:59Z

I think we should merge now. This would make Mapbox GL JS have parity with our mobile SDKs and prevent further accidental violations of our TOS. We can always go back later when the API response contains appropriate flags to indicate permissible white-labeling and configure the control that way.

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

Ok I've updated this PR to rely on the mapbox_logo property returned in the TileJSON object. I'm also in the process of updating how that property is set by the APIs but this PR works independently of those changes.

cc @lucaswoj @jfirebaugh

andrewharvey · 2017-01-27T00:24:11Z

js/ui/map.js

@@ -100,7 +100,7 @@ const defaultOptions = {
 *   Keep in mind that these classes are used for controlling a style layer's paint properties, so are *not* reflected
 *   in an HTML element's `class` attribute. To learn more about Mapbox style classes, read about
 *   [Layers](https://www.mapbox.com/mapbox-gl-style-spec/#layers) in the style specification.
- * @param {boolean} [options.attributionControl=true] If `true`, an [AttributionControl](#AttributionControl) will be added to the map.


Did you really mean to replace attributionControl with logoPosition, or did you mean to replace logoControl with logoPosition?

Given further down you've dropped out options.logoControl with options.logoPosition but retained options.attributionControl.

yipes good catch @andrewharvey -- this was not intentional!

andrewharvey · 2017-01-27T08:31:43Z

js/ui/map.js

@@ -47,6 +48,7 @@ const defaultOptions = {
    hash: false,

    attributionControl: true,
+    logoControl: true,


~~I think you missed updating this to logoPosition: 'bottom-left'.~~ Or actually maybe you just need to remove as it's not used anywhere anymore from what I can see and the default is obtained from .getDefaultPosition() if the option is undefined anyway...

jfirebaugh

🚢 after a few small changes.

jfirebaugh · 2017-01-27T18:28:14Z

js/ui/control/logo_control.js

+class LogoControl {
+
+    constructor() {
+        util.bindAll(['_logoRequired', '_updateLogo'], this);


Nit: _logoRequired doesn't need to be bound (it's not used as an event handler).

jfirebaugh · 2017-01-27T18:29:38Z

js/ui/control/logo_control.js

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


Also needs this._map.off('data', this._updateLogo).

addressed all comments

mollymerp force-pushed the wordmark-control branch 2 times, most recently from 18274a3 to 69fbb02 Compare January 10, 2017 02:02

add mapbox watermark by default

1bb0f0b

mollymerp added the feature 🍏 label Jan 10, 2017

mourner previously requested changes Jan 10, 2017

View reviewed changes

jfirebaugh suggested changes Jan 10, 2017

View reviewed changes

mollymerp force-pushed the wordmark-control branch from 69fbb02 to 299fc3b Compare January 10, 2017 18:46

address feedback, add tests

71ee1c5

mollymerp force-pushed the wordmark-control branch from 299fc3b to 71ee1c5 Compare January 10, 2017 18:58

Add aria-label

dbf00f4

mollymerp force-pushed the wordmark-control branch from 5e7c980 to dbf00f4 Compare January 13, 2017 00:19

lucaswoj added the under development label Jan 18, 2017

mollymerp removed the under development label Jan 25, 2017

mollymerp force-pushed the wordmark-control branch from 13e331d to fa02935 Compare January 26, 2017 22:42

andrewharvey reviewed Jan 27, 2017

View reviewed changes

mollymerp force-pushed the wordmark-control branch from fa02935 to 6abb905 Compare January 27, 2017 03:50

andrewharvey reviewed Jan 27, 2017

View reviewed changes

update logo control to use mapbox_logo flag

7b3ff3e

mollymerp force-pushed the wordmark-control branch from 6abb905 to 7b3ff3e Compare January 27, 2017 18:14

jfirebaugh approved these changes Jan 27, 2017

View reviewed changes

fix binding, turn off event listener onRemove

b81554f

mollymerp merged commit dae12e1 into master Jan 27, 2017

mollymerp deleted the wordmark-control branch January 27, 2017 23:22

andrewharvey mentioned this pull request Feb 16, 2017

collapsed Mapbox workmark on narrow maps #4282

Closed

mollymerp mentioned this pull request Mar 1, 2017

Use mapbox_logo property of TileJSON to determine whether to include logo on map mapbox/mapbox-gl-native#8234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Mapbox wordmark as LogoControl #3933

add Mapbox wordmark as LogoControl #3933

mollymerp commented Jan 10, 2017 •

edited

mourner Jan 10, 2017

mourner Jan 10, 2017

mollymerp Jan 10, 2017

mourner Jan 10, 2017

jfirebaugh left a comment

mollymerp commented Jan 10, 2017

mollymerp commented Jan 11, 2017 •

edited

mourner commented Jan 11, 2017

mollymerp commented Jan 11, 2017 •

edited

andrewharvey commented Jan 12, 2017

mollymerp commented Jan 12, 2017

andrewharvey commented Jan 17, 2017

lucaswoj commented Jan 18, 2017

mollymerp commented Jan 18, 2017 •

edited

mollymerp commented Jan 26, 2017

andrewharvey Jan 27, 2017

mollymerp Jan 27, 2017

andrewharvey Jan 27, 2017 •

edited

jfirebaugh left a comment

jfirebaugh Jan 27, 2017

jfirebaugh Jan 27, 2017

add Mapbox wordmark as LogoControl #3933

add Mapbox wordmark as LogoControl #3933

Conversation

mollymerp commented Jan 10, 2017 • edited

Launch Checklist

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

jfirebaugh left a comment

Choose a reason for hiding this comment

mollymerp commented Jan 10, 2017

mollymerp commented Jan 11, 2017 • edited

mourner commented Jan 11, 2017

mollymerp commented Jan 11, 2017 • edited

andrewharvey commented Jan 12, 2017

mollymerp commented Jan 12, 2017

andrewharvey commented Jan 17, 2017

lucaswoj commented Jan 18, 2017

mollymerp commented Jan 18, 2017 • edited

mollymerp commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewharvey Jan 27, 2017 • edited

Choose a reason for hiding this comment

jfirebaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp commented Jan 10, 2017 •

edited

mollymerp commented Jan 11, 2017 •

edited

mollymerp commented Jan 11, 2017 •

edited

mollymerp commented Jan 18, 2017 •

edited

andrewharvey Jan 27, 2017 •

edited