Skip to content

Alternate Top Banner Instructions#2149

Merged
JThramer merged 76 commits into
masterfrom
kobe/top-banner-instructions-card
Aug 24, 2019
Merged

Alternate Top Banner Instructions#2149
JThramer merged 76 commits into
masterfrom
kobe/top-banner-instructions-card

Conversation

@vincethecoder
Copy link
Copy Markdown
Contributor

@vincethecoder vincethecoder commented Jun 14, 2019

This effort creates a custom top banner instructions a.k.a Instructions Cards. This subscribes to the top banner configured in the route options used in creating a Navigation ViewController.

This top banner instructions component permits customization of the prototype card size, displayed instructions and the desired card style.

TODO

  • Add snapshot tests for cards with maneuver instructions
  • Design review complete
  • Fix broken tests
  • Test drive
    Add snapshot tests for cards with next banner and lane indication(s) instructions
    Provide the ability to customize the data sent to the dataSource

/cc @JThramer @frederoni @1ec5 @d-prukop @okoriep

vincethecoder and others added 30 commits April 22, 2019 13:42
…t might be consumed in refreshing the remaining steps.
…oved commented out code. Refactored hardcoded cell identifier. Deleted the computed extension on Route residing within the card collection.
…uctions-card

# Conflicts:
#	MapboxNavigation/NavigationInteractionDelegate.swift
#	MapboxNavigation/NavigationViewController.swift
#	MapboxNavigation/RouteMapViewController.swift
…uctions-card

# Conflicts:
#	MapboxNavigation/RouteMapViewController.swift
instructionsCardView.updateDistanceFromCurrentLocation(distance)
instructionsCardView.step = step

// TODO: Merge Instructions Card, Lanes & Next Banner View Instructions
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To do: figure out what this to-do means.

Comment thread MapboxNavigation/InstructionsCardContainerView.swift Outdated
@JThramer
Copy link
Copy Markdown
Contributor

Posterity: This PR should be good to merge. Multi-leg support for guidance-cards, tracked in #2176, will be follow-on tail work, and as such all classes in this PR are marked /// :nodoc: until we are ready to announce the new feature.

/cc @mapbox/navigation-ios

@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented Aug 22, 2019

Still getting failures on the Xcode 10.2 / iOS 12.2 CI bot:

/Users/distiller/project/MapboxNavigationTests/GuidanceCardsSnapshotTests.swift:59: error: -[MapboxNavigationTests.GuidanceCardsSnapshotTests testLanesManeuver] : failed - Snapshot comparison failed: Optional(Error Domain=FBSnapshotTestControllerErrorDomain Code=4 "Images different" UserInfo={NSLocalizedFailureReason=image pixels differed by more than 0.00% from the reference image, FBDiffedImageKey=<UIImage: 0x6000013f3b80>, {375, 812}, FBReferenceImageKey=<UIImage: 0x6000013f8690>, {375, 812}, FBCapturedImageKey=<UIImage: 0x6000013f3bf0>, {375, 812}, NSLocalizedDescription=Images different})

The test fixture was generated in Xcode 10.3 with an iOS 12.2 simulator.

@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented Aug 23, 2019

Despite the test fixtures’ file names, the tests are apparently being run on a device other than iPhone X. I see no reason to believe it’d be anything other than iPhone 6 Plus, which is what the CircleCI configuration specifies.

Expected Actual
reference_testLanesManeuver_iPhone_X_Portrait_iOS_12 1@3x failed_testLanesManeuver_iPhone_X_Portrait_iOS_12 1@3x

At the same time, I think it’s unfortunate that the next guidance card would peek in a bit more on one device than another. Either we only want enough of the card to peek in so that the user can tell the cards are swipable, or we want enough to peek in to characterize the following maneuver.

@JThramer JThramer removed the wip label Aug 24, 2019
@JThramer JThramer merged commit 7dc790e into master Aug 24, 2019
@1ec5 1ec5 deleted the kobe/top-banner-instructions-card branch August 24, 2019 03:04
@1ec5 1ec5 added this to the v0.37.0 milestone Aug 24, 2019
@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented Aug 24, 2019

Once we address tail work such as #2175 and #2176, we’ll want to mention this feature and all its API implications in the changelog.

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.

5 participants