Refactoring appearance logic out of RMVC and into Views #993
Conversation
|
@JThramer feel free to break your todo list out into 3 different PRs. |
…up `updateNextBanner`
|
Turned out to be unnecessary @bsudekum. I wasn't able to make as much of an impact on |
JThramer
added
✓ ready for review
and removed
status: in progress
⚠️ DO NOT MERGE
| @@ -71,6 +71,17 @@ open class BaseInstructionsBannerView: UIControl { | |||
| delegate?.didTapInstructionsBanner(self) | |||
| } | |||
|
|
|||
| func update(progress: RouteLegProgress, location: CLLocation, secondsRemaining: TimeInterval) { | |||
There was a problem hiding this comment.
location and secondsRemaining are unused, so setRouteProgress(_:) would suffice as the method name.
| @@ -70,4 +71,9 @@ open class NextBannerView: UIView { | |||
| instructionLabel.rightAnchor.constraint(equalTo: rightAnchor, constant: -16).isActive = true | |||
| } | |||
|
|
|||
| func update(step: RouteStep, instruction: VisualInstruction) { | |||
| maneuverView.step = step | |||
| instructionLabel.instruction = instruction.primaryTextComponents | |||
There was a problem hiding this comment.
An update(list:of:properties:to:update:) method is still a pretty raw wrapper on the previous approach of setting all the properties directly and individually. This is an opportunity for us to think about when RouteMapViewController updates these properties and why. That’ll naturally lead us to a more descriptive name for the method, so we don’t inadvertently call or neglect to call it in the future.
In this case, I noticed that InstructionsBannerView has separate methods for updating the step (as part of setting the route progress) and setting the instruction; why not follow the same pattern for NextBannerView?
There was a problem hiding this comment.
Agreed, noting that maneuverView.step is used for deciding upon the which maneuver arrow to draw.
There was a problem hiding this comment.
Ideally, a developer could just give us a RouteProgress, and we'd be able to draw the correct UI for them.
There was a problem hiding this comment.
Good point. How about making as many of these classes as possible maintain a single RouteProgress-typed property? Each setter would take care of extracting the relevant information from that RouteProgress object. We could create a new protocol for classes that track state via a RouteProgress-typed property.
There was a problem hiding this comment.
Come to think of it, we could have each of these views independently observe routeControllerProgressDidChange notifications. That reduces the need for a controller that manually updates its views.
|
Status update: Yesterday it was agreed among members of @mapbox/navigation-ios that the views associated with the "Top banner" should be extracted into their own containing class to take this concept even further. I am doing this by creating a |
|
|
||
| //update lanes | ||
| if let upComingStep = progress.currentLegProgress?.upComingStep, !progress.currentLegProgress.userHasArrivedAtWaypoint { | ||
| updateLaneViews(step: upComingStep, durationRemaining: progress.currentLegProgress.currentStepProgress.durationRemaining) |
There was a problem hiding this comment.
Investigate why one view uses the secondsRemaining argument and another view seems to pull the same value(?) out of RouteProgress.
JThramer
added
⚠️ DO NOT MERGE
|
This is ultra-stale, and good progress was made in-parallel with the issue that this PR was meant to address. Going to close it. |
This PR concerns #994, where it is desired to refactor view-appearance logic out of the RMVC and into the appropriate views themselves. This makes things a bit more lonely coupled and easier to work with in a piecemeal fashion.
To-Do:
BottomBannerView?(already pretty well-encapsulated)Phase 2 To-Do:
tooFar)/cc @bsudekum @1ec5 @frederoni @ericrwolfe