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

Map Block: Update mapbox-gl dependency to 1.6.1 #14490

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

creativecoder
Copy link
Contributor

@creativecoder creativecoder commented Jan 27, 2020

Fixes #14183

Changes proposed in this Pull Request:

  • Update Mapbox GL js library to 1.6.1

This update opts users into Mapbox's new map load pricing model

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Updates the existing map block

Testing instructions:

  • Insert a map block and add an API key
  • Load the map on the front end of the site
  • When you pan/zoom the map, see that the network requests to get map tiles include a sku value (https://github.com/mapbox/mapbox-gl-js/blob/master/CHANGELOG.md#100)
  • Check the Mapbox statistics for the api key you are using and see that map loads are being tracked (instead of tile loads)

Proposed changelog entry for your changes:

  • Map Block: Update Mapbox GL library to opt into map load based billing

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello creativecoder! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D38195-code before merging this PR. Thank you!

@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 02a7d77

@creativecoder
Copy link
Contributor Author

@jeffersonrabb Checking if you see any issues with upgrading this.

Also, please let us know if there's specific things we should check for the AMP map block.

@creativecoder creativecoder added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Jan 27, 2020
@creativecoder creativecoder requested a review from Copons January 27, 2020 21:39
@creativecoder
Copy link
Contributor Author

I've looked through the Mapbox GL library changelog and don't see anything obvious in our implementation that needs to be updated, but need further testing to be sure.

@creativecoder creativecoder changed the title Map Block: Updates mapbox-gl dependency to 1.6.1 Map Block: Update mapbox-gl dependency to 1.6.1 Jan 27, 2020
@jeherve jeherve added this to the 8.3 milestone Jan 28, 2020
@jeffersonrabb
Copy link
Contributor

Checking if you see any issues with upgrading this.

Looks good to me! I did some before and after testing with a variety of features and everything seems to be working fine.

@jeffersonrabb
Copy link
Contributor

Also, please let us know if there's specific things we should check for the AMP map block.

The AMP implementation is done in a way that has nothing to do with Javascript. Each Map block is rendered as an AMP iFrame, with a src of the current URL with two query parameters conveying post ID and ordinal of the block (e.g. this one is the 3rd Map block in this post). That URL will render a "fake" non-AMP web page containing nothing but the Map block. Therefore, anything that works in a non-AMP context should work the same in AMP, regardless of Javascript changes.

@Copons
Copy link
Contributor

Copons commented Jan 28, 2020

This looks good to me too!
I can't see any noticeable difference than before, and I can see the sku param on the new requests.

Just one thing: I don't see any "map load" tracking stats in my Mapbox account. It might be a matter of time though, but it has been almost an hour now since I started testing.
Scratch that, they finally appeared. 🙂

@creativecoder
Copy link
Contributor Author

Looks like this is ready to go, then, from my perspective. @jeherve Is there anything we should do to update users about the change, besides an entry in the changelog?

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 30, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me and should be good to merge.

A changelog entry should be enough imo.

@jeherve jeherve modified the milestones: 8.3, 8.2 Jan 30, 2020
@dereksmart dereksmart merged commit a7c8375 into master Jan 30, 2020
@dereksmart dereksmart deleted the update/mapbox-gl-dep-to-1.6.1 branch January 30, 2020 16:22
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 30, 2020
@jeherve
Copy link
Member

jeherve commented Jan 30, 2020

Cherry-picked to branch-8.2 in d19765a

@jeherve
Copy link
Member

jeherve commented Jan 30, 2020

r202327-wpcom

jeherve added a commit that referenced this pull request Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map-block API Level - Change to 1.+
7 participants