Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement] Add pluginName=Keplergl url parameter to Mapbox API requests #56

Merged
merged 7 commits into from May 27, 2018

Conversation

ryanbaumann
Copy link
Collaborator

@ryanbaumann ryanbaumann commented May 25, 2018

Adds a URL parameter pluginName=Keplergl to all Mapbox API requests made from the mapboxgl.map object cc/ @heshan0131 @chrisirhc

Example transformed API request:

Known issue

When adding a custom Mapbox style, the map options for transformRequest are not maintained. Map options for transformRequest are maintained when selecting from the list of Kepler.gl styles muted light, light, etc.

@heshan0131 is the map initialization handled for custom Mapbox GL style sheets differently than Uber-sourced styles?

Tests are failing on Travis but pass locally running with yarn run test. Not sure why.

To Do:

  • Determine why the transformRequest map options are not maintained when using a custom map style
  • Fix tests

@ryanbaumann ryanbaumann self-assigned this May 25, 2018
@ryanbaumann ryanbaumann changed the title add Mapbox Keplergl appid to Mapbox API requests Add Keplergl app-id to Mapbox API requests May 25, 2018
@ryanbaumann ryanbaumann changed the title Add Keplergl app-id to Mapbox API requests Add pluginName=Keplergl url parameter to Mapbox API requests May 25, 2018
@chrisirhc
Copy link
Collaborator

Looks like it's lint errors:
image

onViewportChange: updateMap
onViewportChange: updateMap,
transformRequest: (url, resourceType) => {
let transformedUrl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let transformedUrl = url;

// Add parameter to identify kepler.gl Mapbox app traffic
transformedUrl = [url.slice(0, url.indexOf("?")+1), "pluginName=Keplergl&", url.slice(url.indexOf("?")+1)].join('');
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need this else statement here

@heshan0131
Copy link
Contributor

What do you mean by not maintained?

@heshan0131
Copy link
Contributor

I see what you mean now. You should also add the prop to
add-map-style-modal.js

@ryanbaumann
Copy link
Collaborator Author

ryanbaumann commented May 26, 2018

Thanks, @heshan0131 for pointing me in the right direction! I added new props using the same format as in map-container.js into add-map-style-modal.js and confirmed that the URL parameter is added to Mapbox custom styles now. Let me know your final review.

Travis tests seem to be failing for an unrelated reason -- all lint and test commands pass locally. Here's the Travis log -- I'll try pushing again in a few hours and see if it reproduces.

@heshan0131
Copy link
Contributor

travis build is fixed, rebase and it should work now

@ryanbaumann
Copy link
Collaborator Author

@heshan0131 thanks for the help with the NPM changes, all tests are passing now. Apologies for the slightly ugly rebase commits above - they are all duplicates of previous commits except for the last merge master into this branch change.

Ready to merge if you are. Tested on both custom Mapbox map styles, default map styles, and map styles with non-Mapbox-api resources.

@heshan0131
Copy link
Contributor

heshan0131 commented May 27, 2018

@ryanbaumann, I updated the branch to only include your changes, also moved the code to a util

@heshan0131 heshan0131 changed the title Add pluginName=Keplergl url parameter to Mapbox API requests [Enhancement] Add pluginName=Keplergl url parameter to Mapbox API requests May 27, 2018
@heshan0131 heshan0131 merged commit 1a31bf4 into master May 27, 2018
@macrigiuseppe macrigiuseppe deleted the mapbox-app-id branch October 21, 2018 23:37
@chrisirhc chrisirhc mentioned this pull request Feb 25, 2019
bjungs pushed a commit to imec-int/kepler.gl that referenced this pull request Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants