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

Refactor instructions banner #1093

Closed
wants to merge 5 commits into from
Closed

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Feb 8, 2018

This is another take on #1085 using a View Model approach. It also fixes #792

An InstructionsBannerViewModel that handles the logic and an InstructionsBannerViewState which encapsulates the state has been added.

Using a custom instructions banner could look something like:

let viewModel = InstructionsBannerViewModel { (viewModel, state) in
    v.maneuverView.step = state.maneuverViewStep
    v.primaryLabel.instruction = state.primaryInstruction
    v.secondaryLabel.instruction = state.secondaryInstruction
    v.distanceLabel.text = String(state.distanceRemaining)
}
        
viewModel.update(for: routeProgress)

@mapbox/navigation-ios

@bsudekum
Copy link
Contributor

bsudekum commented Feb 8, 2018

I wonder if

let viewModel = InstructionsBannerViewModel { (viewModel, state) in
    v.maneuverView.step = state.maneuverViewStep
    v.primaryLabel.instruction = state.primaryInstruction
    v.secondaryLabel.instruction = state.secondaryInstruction
    v.distanceLabel.text = String(state.distanceRemaining)
}

Is still a lot of domain knowledge for the developer to manage. Could this be simplified even further?

@frederoni
Copy link
Contributor Author

It's actually a pretty common design pattern. As long as we provide examples, I don't think our power users using the custom UI would struggle with this. Not sure how we could simplify this further. 🤔

@JThramer
Copy link
Contributor

JThramer commented Feb 8, 2018

Hmm, I'm not sure I see the benefit of this. It is a more formal pattern, but I don't see why we need to have a formal view model, instead of just offering two methods on InstructionsBannerView.

  • One is a direct setter method, that allows you to set the primaryLabel or secondaryLabel (for example) directly from a string, or instruction, or whatever. If you want the label to say "Willy Wonka's Chocolate Factory", this is what you use.
  • The other is a method that takes the correctly-scoped model-object (say, a RouteLegProgress), and encapsulates the appearance logic (currently in RMVC) to set appropriate values for primaryLabel and secondaryLabel, etc. This is what you use when you just want to feed updates from the routing service (routeController) to the UI components.

If we had multiple routing services, each with their own model schema, then I could totally see the use-case for formalized view-models. But as our codebase stands right now, it just seems like overkill, and tedious overkill at that.

Is there an architectural flaw in #1085 that necessitates this PR?

/cc @bsudekum @1ec5

@bsudekum
Copy link
Contributor

bsudekum commented Feb 9, 2018

In addition to @JThramer, I feel like the amount of code + knowledge added here is pretty high. @frederoni what do you think the drawbacks are to #1085? What I like about #1085 is the simplicity.

Copy link
Contributor

@JThramer JThramer left a comment

Choose a reason for hiding this comment

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

Going to mark this "Request Changes" until we pow-wow about this PR's fate.

@frederoni frederoni closed this Feb 13, 2018
@frederoni frederoni deleted the fred-extract-instr-banner branch February 13, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tweak banner spacing
3 participants