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

Update to Mapbox Maps 10.0.0-beta.20, Navigation Native 50.0.0 and Common 12.0.0. #3010

Merged
merged 19 commits into from May 26, 2021

Conversation

MaximAlien
Copy link
Contributor

@MaximAlien MaximAlien commented May 17, 2021

Description

PR updates Mapbox Maps to 10.0.0-beta.20 (pointing to main for now), Navigation Native 50.0.0 and Common 12.0.0.

@MaximAlien MaximAlien added build Issues related to builds and dependency management. UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. labels May 17, 2021
@MaximAlien MaximAlien added this to the v2.0.0 (RC) milestone May 17, 2021
@MaximAlien MaximAlien requested a review from a team May 17, 2021 09:58
@MaximAlien MaximAlien self-assigned this May 17, 2021
@MaximAlien MaximAlien marked this pull request as draft May 17, 2021 09:58
@MaximAlien MaximAlien marked this pull request as ready for review May 17, 2021 11:23
@MaximAlien MaximAlien force-pushed the maxim/update-mapbox-maps-dependency branch from 2fc4328 to d0bb78f Compare May 18, 2021 11:17
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Looks pretty reasonable to me. Just a question about control flow in one of the updated methods. (Remember to update to v2.0.0-beta.20 once it’s out.)

var layerPosition: MapboxMaps.LayerPosition? = nil

if isMainRoute {
if let aboveLayerIdentifier = mapView.mainRouteLineParentLayerIdentifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the identifier is nil, this method silently returns without adding a layer or printing to the console. Is that intentional? It’d be easier to keep track of cases like this by making layerPosition constant (but still optional).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code to use let instead of var. Otherwise code should work fine: if there is no layerPosition layer will be added at the top of the stack.

@MaximAlien MaximAlien force-pushed the maxim/update-mapbox-maps-dependency branch from b1315b2 to b259fbe Compare May 20, 2021 05:25
@MaximAlien MaximAlien changed the title Update Mapbox Maps dependency to the most recent changes from main. Update to Mapbox Maps 10.0.0-beta.20, Navigation Native 50.0.0 and Common 12.0.0. May 20, 2021
@MaximAlien MaximAlien requested a review from a team May 20, 2021 07:24
@@ -72,22 +72,13 @@ class Navigator {
Restrict direct initializer access.
*/
private init() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there will be a lot of conflicts with #3014. cc @Udumft

@MaximAlien MaximAlien force-pushed the maxim/update-mapbox-maps-dependency branch from 8c0ebc7 to f20b0be Compare May 21, 2021 11:39
@MaximAlien MaximAlien force-pushed the maxim/update-mapbox-maps-dependency branch from 856fae6 to 3fe773a Compare May 25, 2021 11:44
@MaximAlien MaximAlien force-pushed the maxim/update-mapbox-maps-dependency branch from 3fe773a to 684a23e Compare May 25, 2021 17:02
@MaximAlien MaximAlien merged commit 2fc35de into release-v2.0 May 26, 2021
@MaximAlien MaximAlien deleted the maxim/update-mapbox-maps-dependency branch May 26, 2021 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to builds and dependency management. UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants