fix update edit link, add query parameters for map feedback #4685

mollymerp · 2017-05-05T19:10:43Z

This PR does two things:

fix the edit link that we didn't know was broken as a result of Use stylelint to enfore css prefixes #4381 – the HTML for the map feedback link comes from the TileJSON which mapbox-gl-js does not control, so when I changed the mapboxgl prefix for the queryselector class that modifies the edit link it stopped getting updated, and the test didn't catch it bc we provide the TileJSON html inside the test.
adds query parameters for Including a username in map-feedback attribution link #3558

an alternative to this approach would be editing the tilejson in place to abide by our mapboxgl- prefix requirement.

closes #3558

cc @tristen

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

mollymerp · 2017-05-05T19:15:13Z

@tristen can you let me know if these query params (owner and id) are what you're looking for?
Scott confirmed that just the style id is not adequate to find the style associated bc ids are not globally unique, but owner+id pairs are.

tristen · 2017-05-06T17:44:38Z

Ahh thanks for the clarification on owner. What you are capturing looks great to me! 🎉

tristen · 2017-05-09T15:10:24Z

test/unit/ui/control/attribution.test.js

        map.on('data', (e) => {
            if (e.dataType === 'source' && e.sourceDataType === 'metadata') {
-                t.equal(attribution._editLink.href, 'https://www.mapbox.com/map-feedback/#/0/0/1', 'edit link contains map location data');
+                t.equal(attribution._editLink.href, 'https://www.mapbox.com/map-feedback/#/0/0/0?owner=mapbox&id=streets-v10', 'edit link contains map location data');


I think we can just omit these params if owner or id isn't set rather than passing defaults.

these aren't defaults – I set them up at L17-18 😛

omg I'm reading through the test 😭 😭 😭 😭

tristen · 2017-05-09T15:57:07Z

test/unit/ui/control/attribution.test.js

                map.setZoom(2);
-                t.equal(attribution._editLink.href, 'https://www.mapbox.com/map-feedback/#/0/0/3', 'edit link updates on mapmove');
+                t.equal(attribution._editLink.href, 'https://www.mapbox.com/map-feedback/#/0/0/2?owner=mapbox&id=streets-v10', 'edit link updates on mapmove');


One thing that needs to change! hash should follow params. Structure should look like this:

https://www.mapbox.com/feedback/?owner=mapbox&id=streets-v10#/0/0/2

tristen · 2017-05-09T15:58:23Z

@mollymerp one last thing too: The path should change from map-feedback to feedback.

mollymerp · 2017-05-12T17:19:54Z

@tristen I made the changes you requested. let us know when you need the release published by.

anandthakker · 2017-05-12T18:09:49Z

src/ui/control/attribution_control.js

@@ -66,11 +66,12 @@ class AttributionControl {
    }

    _updateEditLink() {
-        if (!this._editLink) this._editLink = this._container.querySelector('.mapboxgl-improve-map');
+        if (!this._editLink) this._editLink = this._container.querySelector('.mapbox-improve-map');


So, if I'm understanding this right, the change from .mapboxgl-improve-map to .mapbox-improve-map is in order to match against the CSS class used in our the TileJSON provided by Mapbox's vector tile sources?

correct @anandthakker - I will ticket out changing that css class at the API level separately

anandthakker · 2017-05-12T18:11:39Z

src/ui/control/attribution_control.js

-                    center.lng}/${center.lat}/${Math.round(this._map.getZoom() + 1)}`;
+            const styleParams = (this.styleOwner && this.styleId) ? `?owner=${this.styleOwner}&id=${this.styleId}` : '';
+            this._editLink.href = `https://www.mapbox.com/feedback/${styleParams}#/${
+                    Math.round(center.lng * 1000) / 1000}/${Math.round(center.lat * 1000) / 1000}/${Math.round(this._map.getZoom())}`;


Why are we dropping the +1 from this._map.getZoom() + 1? Was it incorrect in the first place?

@anandthakker GL and raster zoom levels are off by one 😊. the old map-feedback link used Mapbox.js and therefore we had to add a zoom level to the hash for the link to open at the right spot.

Is it still necessary to round the zoom level?

1ec5 · 2017-05-17T16:24:15Z

src/ui/control/attribution_control.js

-            this._editLink.href = `https://www.mapbox.com/map-feedback/#/${
-                    center.lng}/${center.lat}/${Math.round(this._map.getZoom() + 1)}`;
+            const styleParams = (this.styleOwner && this.styleId) ? `?owner=${this.styleOwner}&id=${this.styleId}` : '';
+            this._editLink.href = `https://www.mapbox.com/feedback/${styleParams}#/${


If this map is using a custom style designed in Studio (as opposed to a default style), should the feedback tool display that style in its map? If so, we'll also need to pass in an access token or somehow make it so that the feedback tool can access any style.

/cc @tristen

@1ec5 yikes I think you brought up something pretty critical here. I assumed a token could be generated with the scope to include any custom style but that doesn't appear to be the case. It looks like an access token would need to be additionally passed for this to work.

@mollymerp client is encoded in the token so it might make sense to drop that parameter for token instead?

mollymerp · 2017-05-23T07:46:55Z

@tristen Added access_token param!

1ec5 · 2017-05-23T08:49:20Z

Ported to iOS and macOS in mapbox/mapbox-gl-native#9078.

tristen · 2017-05-23T20:13:40Z

@mollymerp I don't want to hold this PR much longer but we can we also capture bearing + pitch at the end of the hash?

mollymerp · 2017-05-23T23:11:49Z

@tristen added it. note that the previous order I was using in the hash was different and also had an extra / compared to what we use as the existing map hash option in gl-js. (#/lat/lng/zoom vs #zoom/lat/lng) 😬 I imagine this was due to how the original map-feedback page was implemented. reverted per tristen's request

with the most recent change the new feedback page and mapbox gl hashes should remain consistent. let me know if this is 👌

mollymerp · 2017-05-24T09:54:55Z

I made some decent changes here so @anandthakker if you want to take another 👀 that would probably be a good idea 😊

anandthakker · 2017-05-24T18:16:27Z

src/ui/control/attribution_control.js

@@ -69,7 +70,7 @@ class AttributionControl {
        if (!this._editLink) this._editLink = this._container.querySelector('.mapbox-improve-map');
        if (this._editLink) {
            const center = this._map.getCenter();
-            const styleParams = (this.styleOwner && this.styleId) ? `?owner=${this.styleOwner}&id=${this.styleId}` : '';
+            const styleParams = (this.styleOwner && this.styleId) ? config.ACCESS_TOKEN ? `?owner=${this.styleOwner}&id=${this.styleId}&access_token=${config.ACCESS_TOKEN}` : `?owner=${this.styleOwner}&id=${this.styleId}` : '';


config.ACCESS_TOKEN wouldn't pick up an access token that a user set using mapboxgl.accessToken = ... right?

@anandthakker I think it should

mapbox-gl-js/src/index.js

Line 50 in 78fc9c4

set: function(token) { config.ACCESS_TOKEN = token; }

anandthakker · 2017-05-24T20:45:33Z

src/ui/map.js

@@ -192,9 +192,9 @@ class Map extends Camera {

        bindHandlers(this, options);

-        this._hash = options.hash && (new Hash()).addTo(this);
+        this.hash = options.hash && (new Hash()).addTo(this);


IMO we should keep the _ to signal that this is a private member rather than a public API.

oooh good point – for some reason I was thinking the convention was no _ if its called from w/in another module but that is not correct 😊

mollymerp · 2017-05-29T09:54:01Z

We good to pull the trigger on this @tristen @anandthakker ?

tristen · 2017-05-29T11:57:18Z

@mollymerp Looks good to merge from my end!

anandthakker

👍

fix update edit link, add query parameters for map feedback

973fe84

mollymerp force-pushed the feedback-info branch from 23e96c9 to 973fe84 Compare May 5, 2017 21:23

tristen reviewed May 9, 2017

View reviewed changes

tristen requested changes May 9, 2017

View reviewed changes

update url structure

d18d408

mollymerp added the release blocker ⛔ label May 12, 2017

correct url in test

ef0eb1f

anandthakker reviewed May 12, 2017

View reviewed changes

1ec5 mentioned this pull request May 13, 2017

Migrate to GL JS–powered feedback form mapbox/mapbox-gl-native#8988

Closed

1ec5 reviewed May 17, 2017

View reviewed changes

add access_token param

ad82f4f

mollymerp force-pushed the feedback-info branch from c13c3e9 to ad82f4f Compare May 23, 2017 07:51

1ec5 mentioned this pull request May 23, 2017

Migrate to GL JS–powered feedback form mapbox/mapbox-gl-native#9078

Merged

update tests w access_token param

cbfe7b7

tristen approved these changes May 23, 2017

View reviewed changes

tristen removed the release blocker ⛔ label May 23, 2017

Molly Lloyd added 2 commits May 24, 2017 00:38

add pitch and bearing

dc8664f

fix hash, make consistent w existing hash option

e2d0a3f

mollymerp force-pushed the feedback-info branch from 00a7082 to e2d0a3f Compare May 23, 2017 23:08

use rounding instead of toFixed to get rid of .00s, update tests

8601854

make hash conform to feedback expectations

fc2afc1

mollymerp force-pushed the feedback-info branch from 853a19a to fc2afc1 Compare May 24, 2017 16:56

mollymerp mentioned this pull request May 24, 2017

Change hash to be in lng,lat order #4748

Closed

anandthakker reviewed May 24, 2017

View reviewed changes

mollymerp force-pushed the feedback-info branch from 3434703 to c87fbae Compare May 29, 2017 15:51

change hash back to private property

192dfd4

mollymerp force-pushed the feedback-info branch from c87fbae to 192dfd4 Compare May 29, 2017 15:52

anandthakker approved these changes May 30, 2017

View reviewed changes

mollymerp merged commit 7d2b30b into master May 31, 2017

mollymerp deleted the feedback-info branch May 31, 2017 09:46

mollymerp mentioned this pull request Jun 6, 2017

Include map style id and owner in map feedback link #4669

Closed

jmunowitch mentioned this pull request Sep 14, 2017

Upgrade to Mapbox-gl-js 0.40.0 archesproject/arches#2410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix update edit link, add query parameters for map feedback #4685

fix update edit link, add query parameters for map feedback #4685

mollymerp commented May 5, 2017

mollymerp commented May 5, 2017

tristen commented May 6, 2017 •

edited

Loading

tristen May 9, 2017

mollymerp May 9, 2017

tristen May 9, 2017

tristen May 9, 2017 •

edited

Loading

tristen commented May 9, 2017

mollymerp commented May 12, 2017

anandthakker May 12, 2017

mollymerp May 12, 2017

anandthakker May 12, 2017

mollymerp May 12, 2017 •

edited

Loading

mollymerp May 12, 2017

1ec5 May 23, 2017

1ec5 May 17, 2017

tristen May 17, 2017

mollymerp commented May 23, 2017

1ec5 commented May 23, 2017

tristen commented May 23, 2017

mollymerp commented May 23, 2017 •

edited

Loading

mollymerp commented May 24, 2017

anandthakker May 24, 2017

mollymerp May 24, 2017

anandthakker May 24, 2017

anandthakker May 24, 2017

mollymerp May 24, 2017

mollymerp commented May 29, 2017

tristen commented May 29, 2017

anandthakker left a comment

fix update edit link, add query parameters for map feedback #4685

fix update edit link, add query parameters for map feedback #4685

Conversation

mollymerp commented May 5, 2017

Launch Checklist

mollymerp commented May 5, 2017

tristen commented May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristen May 9, 2017 • edited Loading

Choose a reason for hiding this comment

tristen commented May 9, 2017

mollymerp commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp May 12, 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

mollymerp commented May 23, 2017

1ec5 commented May 23, 2017

tristen commented May 23, 2017

mollymerp commented May 23, 2017 • edited Loading

mollymerp commented May 24, 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

mollymerp commented May 29, 2017

tristen commented May 29, 2017

anandthakker left a comment

Choose a reason for hiding this comment

tristen commented May 6, 2017 •

edited

Loading

tristen May 9, 2017 •

edited

Loading

mollymerp May 12, 2017 •

edited

Loading

mollymerp commented May 23, 2017 •

edited

Loading