Add overview button by bsudekum · Pull Request #106 · mapbox/mapbox-navigation-ios

bsudekum · 2017-03-29T21:54:14Z

Adds an overview button which fits the user location to the end of the route on all location updates

todo:

style the button
allow for recenter button to show while panning in overview mode
i think the camera animations could be improved a bit

1ec5 · 2017-03-30T10:54:40Z

MapboxNavigationUI/RouteMapViewController.swift

If you ever have to check for nil, that’s a sign you should make the variable optional (?) instead of implicitly unwrapped optional (!). Then you can simply call resetTrackingModeTimer?.invalidate() below.

1ec5 · 2017-03-30T10:58:27Z

MapboxNavigationUI/RouteMapViewController.swift

I agree that this method is useful, but you don’t appear to be using it anywhere. Looks like you went with passing in a raw array of coordinates in order to more easily get a subarray of them.

1ec5 · 2017-03-30T10:58:55Z

MapboxNavigationUI/RouteMapViewController.swift

Move this extension to a separate file to make it more discoverable.

bsudekum · 2017-04-04T18:00:20Z

Updated with feedback and design

1ec5

This looks reasonable from a technical perspective. I’m a little concerned that the hit target is too small and too close to the Cancel button, and that residential roads may bleed into the button’s white background.

/cc @mayagao

1ec5 · 2017-04-04T23:57:19Z

MapboxNavigationUI/RouteMapViewController.swift

+        if isInOverviewMode {
+            recenterButton.isHidden = true
+            overviewButton.setTitle("Overview", for: .normal)
+            isInOverviewMode = !isInOverviewMode


It would be clearer to either move this line out of the if statement (and remove the similar one from the else case) or explicitly say false here and true below.

bsudekum · 2017-04-27T20:59:57Z

Made some more headway. Still todo:

adopt standard tint for overview button
adopt standard tint for resume button text

frederoni · 2017-04-27T21:15:00Z

MapboxNavigation/Resources/Navigation.storyboard

-                                <state key="normal" image="location"/>
+                                <inset key="contentEdgeInsets" minX="0.0" minY="0.0" maxX="10" maxY="0.0"/>
+                                <inset key="titleEdgeInsets" minX="15" minY="0.0" maxX="0.0" maxY="0.0"/>
+                                <state key="normal" title="Resume" image="location">


Should we localize this?

frederoni · 2017-04-27T21:29:28Z

adopt standard tint for overview button

If the image asset is set to render as template image and the button type is MBButton it should just work.

adopt standard tint for resume button text

Is slightly more tricky since not all buttons should have tinted image and text.

I would:
Create a highlighted button class in the style.

@objc(MBHighlightedButton)
public class HighlightedButton: Button { }

and apply a textColor for all labels that are contained inside a HighlightedButton

UILabel.appearance(for: traitCollection, whenContainedInInstancesOf: [HighlightedButton.self]).textColor = color

bsudekum · 2017-04-28T00:20:40Z

@frederoni having a hard time getting the style of the button customizable. Could you take a look at this?

frederoni · 2017-04-28T12:09:29Z

Added MBHighlightedButton that only supports tint and text color so far. Let's extend customizability as the design solidifies.

bsudekum · 2017-04-28T18:04:02Z

@frederoni @1ec5 this is ready for another round of review

bsudekum · 2017-04-28T18:29:42Z

Crappy errors on bitrise:

*** Cloning osrm-text-instructions.swift
*** Cloning aws-sdk-ios
*** Cloning Pulley
*** Cloning MapboxDirections.swift
*** Skipped downloading aws-sdk-ios.framework binary due to the error:
	"API rate limit exceeded for 208.52.166.154. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)"
*** Checking out aws-sdk-ios at "1a8432b03c22326fb7ed86fac978212106e2d465"
*** Downloading binary-only framework Mapbox-iOS-SDK at "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json"
*** Downloading Mapbox-iOS-SDK.framework binary at "3.5.2"
*** Skipped downloading Polyline.framework binary due to the error:
	"API rate limit exceeded for 208.52.166.154. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)"
*** Checking out Polyline at "v4.1.1"
*** Skipped downloading MapboxDirections.swift.framework binary due to the error:
	"API rate limit exceeded for 208.52.166.154. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)"
*** Checking out MapboxDirections.swift at "v0.9.0"
*** Skipped downloading osrm-text-instructions.swift.framework binary due to the error:
	"API rate limit exceeded for 208.52.166.154. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)"
*** Checking out osrm-text-instructions.swift at "v0.1.1"
*** Skipped downloading Pulley.framework binary due to the error:
	"API rate limit exceeded for 208.52.166.154. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)"
*** Checking out Pulley at "1.3.1"
*** Skipped downloading SDWebImage.framework binary due to the error:
	"API rate limit exceeded for 208.52.166.154. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)"
*** Checking out SDWebImage at "4.0.0"
A shell task (/usr/bin/env git checkout --quiet 3d97bb75144147e47db278ec76e5e70c6b2243db) failed with exit code 128:
fatal: reference is not a tree: 3d97bb75144147e47db278ec76e5e70c6b2243db

frederoni · 2017-04-28T18:31:42Z

~~Did caching stop working? Update carthage and see if you get any changes in Cartfile.resolved, if so, commit it.~~

-edited-
Oh, I get the same locally using the latest version of Carthage and our Bitrise workflow always upgrades to the latest version.

frederoni · 2017-04-28T19:26:31Z

Crappy errors on bitrise:

Seems to be a mirror of libwebp (SDWebImage dependency) that disappeared from GH. (https://github.com/webmproject/libwebp)

bsudekum · 2017-04-28T19:28:16Z

SDWebImage/SDWebImage#1881

frederoni · 2017-04-28T21:30:35Z

Cartfile.resolved

@@ -1,5 +1,5 @@
 binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" "3.5.2"
-github "52inc/Pulley" "1.4"
+github "52inc/Pulley" "1.1"


What happened here?

oy, bad merge. Will fix

bsudekum · 2017-05-02T19:47:02Z

Note, when in overview mode, I'm hiding the current way name label as not to clutter the screen.

bsudekum · 2017-05-03T16:50:13Z

@1ec5 @frederoni bump, review here would be 💯

frederoni · 2017-05-03T18:36:07Z

@bsudekum Can you rebase this PR before review?

bsudekum · 2017-05-03T21:36:55Z

@frederoni the rebase is looking pretty rough. the branch is up to date with master however.

1ec5 · 2017-05-04T00:47:28Z

MapboxNavigation/Resources/Navigation.storyboard

+                                <fontDescription key="fontDescription" type="system" pointSize="16"/>
+                                <inset key="contentEdgeInsets" minX="0.0" minY="0.0" maxX="10" maxY="0.0"/>
+                                <inset key="titleEdgeInsets" minX="15" minY="0.0" maxX="0.0" maxY="0.0"/>
+                                <state key="normal" title="Resume" image="location">


This needs to be localized. In the File inspector, click the Make Localizable button. When we merge this PR, we'll need to add Navigation.strings to Transifex and set up autoupdating.

1ec5 · 2017-05-04T00:55:28Z

MapboxNavigation/RouteMapViewController.swift

+        guard polyline.overlayBounds.ne - polyline.overlayBounds.sw > 200 else {
+            return
+        }
+        mapView.setVisibleCoordinateBounds(polyline.overlayBounds, edgePadding: overviewContentInset, animated: true)


FYI, the map SDK has a "targeted course tracking" feature that accomplishes a similar effect. You keep the userTrackingMode as followWithCourse but also set targetCoordinate. The map automatically adjusts the visible coordinate bounds to keep both the current location and target coordinate in view at the same time. What's nice is that the user puck continues to show (and point in the right direction).

Nevertheless, I think this code is fine for now, because targeted course tracking mode doesn't reset to north (which is arguably better) and may not show the entire route line (if part of the route bows way off to the side or behind the user.

Bridge profile identifier to Objective-C

bsudekum requested review from 1ec5 and frederoni March 29, 2017 21:54

1ec5 reviewed Mar 30, 2017

View reviewed changes

Add overview button

1e8a097

bsudekum force-pushed the overview-button branch from 7822d17 to 1e8a097 Compare March 31, 2017 16:06

bsudekum added 2 commits April 4, 2017 09:18

pad

d5fe276

Make optional, move

dd1b142

1ec5 approved these changes Apr 5, 2017

View reviewed changes

Bobby Sudekum added 3 commits April 26, 2017 12:38

Merge branch 'master' into overview-button

2f9e994

update

3712bd7

Improve

f5022a4

frederoni reviewed Apr 27, 2017

View reviewed changes

Bobby Sudekum added 3 commits April 27, 2017 14:39

fix

86a73e0

fix up

b6e6558

Merge branch 'master' into overview-button

f08304c

Stylable overview button

d99b011

Bobby Sudekum added 2 commits April 28, 2017 09:53

add

a003e33

animate

629e7ed

Merge branch 'master' into overview-button

d31120a

frederoni reviewed Apr 28, 2017

View reviewed changes

Bobby Sudekum added 5 commits April 28, 2017 14:37

fix version

4ab9cd1

add back HighlightedLabel

06dc837

Merge branch 'master' into overview-button

a56e1d7

Merge branch 'master' into overview-button

ce4a7bc

remove

71457a2

Merge branch 'master' into overview-button

5d0ba85

bsudekum changed the title ~~Add overview button [wip]~~ Add overview button May 3, 2017

Dont keep zooming in as user approaches endpoint

8b80c4a

1ec5 approved these changes May 4, 2017

View reviewed changes

1ec5 mentioned this pull request May 4, 2017

Adopt Transifex for localizations #203

Merged

add

1bf6b5c

bsudekum merged commit 0f8cf74 into master May 4, 2017

bsudekum deleted the overview-button branch May 4, 2017 16:05

bsudekum mentioned this pull request May 11, 2017

How to access various map features #218

Closed

wishtrip-dev pushed a commit to wishtrip-dev/mapbox-navigation-ios that referenced this pull request Feb 28, 2018

Merge pull request mapbox#106 from mapbox/fred-objc-compatibility

c15cdb4

Bridge profile identifier to Objective-C

Comments

Conversation

bsudekum commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Apr 4, 2017

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Apr 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni commented Apr 27, 2017

Uh oh!

bsudekum commented Apr 28, 2017

Uh oh!

frederoni commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsudekum commented Apr 28, 2017

Uh oh!

bsudekum commented Apr 28, 2017

Uh oh!

frederoni commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frederoni commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsudekum commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented May 2, 2017

Uh oh!

bsudekum commented May 3, 2017

Uh oh!

frederoni commented May 3, 2017

Uh oh!

bsudekum commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bsudekum commented Mar 29, 2017 •

edited

Loading

frederoni commented Apr 28, 2017 •

edited

Loading

frederoni commented Apr 28, 2017 •

edited

Loading

frederoni commented Apr 28, 2017 •

edited

Loading