-
Notifications
You must be signed in to change notification settings - Fork 310
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
Move UI updates methods to class #1085
Conversation
|
||
guard let visualInstruction = routeProgress.currentLegProgress.currentStep.instructionsDisplayedAlongStep?.last else { return } | ||
|
||
set(visualInstruction.primaryTextComponents, secondaryInstruction: visualInstruction.secondaryTextComponents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, this set(_:secondaryInstruction:)
method again. Can we break it out into two properties that can be set independently? If we really want to prevent one from being set without the other, then we should replace set(_:secondaryInstruction:)
with a update(for:)
method that takes a single VisualInstruction.
/** | ||
Updates the instructions banner for a given `RouteProgress`. | ||
*/ | ||
public func update(for routeProgress: RouteProgress) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like routeProgress
is used only for its currentLegProgress
, so make this method take a RouteLegProgress instead.
My suggestion here (and the original idea behind #993) is to encapsulate the logic that determines whether the next / lanes / status / whatever UI component should show in it's own view container. That way we have a nice encapsulated class that does nothing but determine the visibility of it's children (and ideally, pass messages to them), and users who want only the "top banner" part of our UI have a nice clean class to hook-up to their custom UI. |
Cartfile
Outdated
@@ -1,5 +1,5 @@ | |||
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 3.7 | |||
github "mapbox/MapboxDirections.swift" ~> 0.16.0 | |||
github "mapbox/MapboxDirections.swift" "banner-types" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frederoni noting, I updated this branch.
|
||
let usePreviousLeg = indexPath.section != 0 && indexPath.row == 0 | ||
let leg = routeProgress.route.legs[indexPath.section] | ||
let arrivalSecondaryInstruction = leg.destination.name | ||
|
||
if usePreviousLeg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JThramer did a nice refactor of this if statement in https://github.com/mapbox/mapbox-navigation-ios/pull/1063/files#diff-05dcf05089a2947433b80faae897f68dR207. Wonder if we could reuse this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some minor nit-picks.
/** | ||
Updates the instructions banner for a given `RouteProgress`. | ||
*/ | ||
public func update(for currentLegProgress: RouteLegProgress) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this method only cares about currentLegProgress.currentStepProgress
, it would be better if this method took a RouteStepProgress
argument directly.
Dealer's choice though, I will be tackling this (and things like this) directly when I resurrect #993.
guard isHidden == true else { return } | ||
if animated { | ||
UIView.defaultAnimation(0.3, animations: { | ||
self.isHidden = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work for animation. You have to animate the alpha
channel, not the boolean.
Fun fact: isHidden
automatically sets to true
when the alpha
value has sufficiently approached 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isHidden
is animatable if the super view is a UIStackView and auto layout is being used. (https://medium.com/@nrewik/easy-animation-with-uistackview-8878b2856ae2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I guess that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, TIL!
@@ -21,26 +21,26 @@ public class ManeuverView: UIView { | |||
} | |||
} | |||
|
|||
@objc public var step: RouteStep? { | |||
@objc public var isStart = false { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These property names aren't super-descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but we should pin to an actual release of MapboxDirections before merging.
This PR should make it easier for developers who are creating their own UI to update the top banner components accordingly.
todo:
/cc @mapbox/navigation-ios