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

Designable navigation view #981

Merged
merged 26 commits into from
Jan 18, 2018
Merged

Designable navigation view #981

merged 26 commits into from
Jan 18, 2018

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Jan 8, 2018

This PR adds a new NavigationView which will replace the RouteMapViewController’s view that's currently laid-out in the storyboard. A designable view of the whole UI encapsulating all other views.

Hooking it up to RouteMapViewController will be done in a subsequent PR so this change alone won't effectively change anything aside from:

  • WayNameView merged into WayNameLabel
  • Renamed LanesContainerView to LanesView

screenshot 2018-01-10 17 11 22

@JThramer @bsudekum 👀

@frederoni frederoni added the ⚠️ DO NOT MERGE PR should not be merged! label Jan 8, 2018
@frederoni frederoni changed the title Refactor RMVC Refactor RMVC view Jan 9, 2018
@JThramer
Copy link
Contributor

JThramer commented Jan 9, 2018

Awesome. Looks great so far!

@frederoni frederoni changed the title Refactor RMVC view Designable navigation view Jan 10, 2018
@frederoni frederoni removed the ⚠️ DO NOT MERGE PR should not be merged! label Jan 11, 2018
@frederoni
Copy link
Contributor Author

frederoni commented Jan 12, 2018

This PR is ready for review. As noted in the description, this change alone won't change anything aside from:

  • WayNameView merged into WayNameLabel
  • Renamed LanesContainerView to LanesView

@bsudekum bsudekum added this to the v0.13.0 milestone Jan 12, 2018
public override func prepareForInterfaceBuilder() {
super.prepareForInterfaceBuilder()

for _ in 0...4 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there are always 4?

Copy link
Contributor Author

@frederoni frederoni Jan 12, 2018

Choose a reason for hiding this comment

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

Yep, this is only being used when rendering in Interface Builder so 5 mock lanes.

*/
@IBDesignable
@objc(MBNavigationView)
open class NavigationView: UIView {
Copy link
Contributor

Choose a reason for hiding this comment

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

This namespace is really starting to get crowded 🙈.

weak var muteButton: FloatingButton!
weak var reportButton: FloatingButton!
weak var rerouteReportButton: ReportButton!
weak var separatorView: SeparatorView!
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are all these variables used?

@bsudekum
Copy link
Contributor

Initial test feels good. Interested to hear feedback from @1ec5 & @JThramer too.

weak var informationStackView: UIStackView!
// Vertically laid-out stack view below the information stack view ontop of the map view, docked
// to the top right, consisting of Overview, Mute, and Report button.
weak var floatingStackView: UIStackView!
Copy link
Contributor

@JThramer JThramer Jan 17, 2018

Choose a reason for hiding this comment

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

The one major comment I have on this PR is this: Since this is no longer IB-based, it would be much cleaner to use a lazy var pattern to set up all of these child views, like so:

lazy var floatingStackView: UIStackView = { 
    let stack = UIStackView()
    stack.axis = .vertical
    stack.distribution = .equalSpacing
    stack.spacing = 8
    stack.translatesAutoresizingMaskIntoConstraints = false
    return stack
} ()

then, setupViews() becomes this:

func setupViews() {
    [..., floatingStackView, overviewButton, muteButton, reportButton, ...].forEach(addSubview(_:))
}

However, this does work for now, so if we want to just get this across the line then I'll happily take care of it in my PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree about the lazy-loaded ivars. Even more if we decide to lazy-load localized strings. However, I'm going to merge this as is b/c the utilization of this class will be done in a subsequent PR which is when we can measure the impact.

@bsudekum bsudekum requested a review from 1ec5 January 17, 2018 18:41
@frederoni frederoni merged commit 3acf2a4 into master Jan 18, 2018
@frederoni frederoni deleted the refactor-rmvc branch January 18, 2018 11:31
@JThramer JThramer mentioned this pull request Feb 22, 2018
40 tasks
@1ec5 1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018
@1ec5 1ec5 added this to the v0.13.0 milestone Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible changes that break backwards compatibility of public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants