Adding generic shield support to instruction banner. #1417

JThramer · 2018-05-11T00:31:11Z

Adding support for generic shields. Implements #1190. This also has the neat side-effect of rendering normal shields as generic ones until their images load.

To-Do:

Fix failing tests
Add Testing
public-ation
revert upstream dependency
Changelog entry
Document GenericRouteShield
Cache GenericRouteShield based upon it's foreground color / text
Remove default case from VisualInstructionComponent extension

/cc @mapbox/navigation-ios

1ec5 · 2018-05-11T00:33:11Z

MapboxNavigation/GenericRouteShield.swift

+import Foundation
+import UIKit
+
+class GenericRouteShield: StylableView {


Should this class be public, so that developers can customize the colors or roundedness?

1ec5 · 2018-05-11T00:34:59Z

MapboxNavigation/InstructionPresenter.swift

+                } else if let genericShieldString = attributedString(forGenericShield: component, dataSource: dataSource) {
+                    build(component, [joinString, genericShieldString])
+                } else {
+                    fallthrough


This is a clever use of a switch statement, but do you think the fallthroughs inside conditionals make the code flow a little more difficult to keep track of?

@1ec5 It's certainly a complex switch, but I'd make the case that it's a heck of a lot easier to follow than the mess of if statements before it.

I like to think that the fallthrough statements enforce the idea that everything falls-back to text.

Would it make sense to factor this switch out into it's own function? This is function is getting boarder-line large.

…g test failures.

bsudekum · 2018-05-14T16:34:59Z

This is looking great!

JThramer · 2018-05-15T23:06:12Z

Posterity: determined that test failures are not due to machine. Bisected work on this branch to find root-cause of test issues. First bad commit found to be fd2a57b37b6d0ce8504dc8468e464de35b5a7cbc. Not super-helpful (lots of stuff happened in that commit), but it's the first step in helping me narrow down the bad behavior.

…sing components are shields

…xpecting to see a green light.

…cted, and that they are replaced by actual images as we receive them. This exposes a bug where we're having cache key issues for generic shields, as the cache is storing the generated generic attachment image as the loaded attachment image's key-value. Also: Tightening up the timeout values now that things are looking pretty stable.

…are always represented by generic cache keys.

…clause

…Images_64".

…ity to tune tolerance per-test.

bsudekum · 2018-05-18T17:42:28Z

MapboxNavigationTests/InstructionsBannerViewSnapshotTests.swift

        view.frame.size = size
-        FBSnapshotVerifyView(view, tolerance: 0.01)
+        FBSnapshotVerifyView(view, suffixes: ["_64"], tolerance: tolerance)


This looks suspect.

The suffixes: ["_64"]? That just tells the snapshot framework that we only have reference images in the ReferenceImages_64 folder, so it doesn't try to look in the ReferenceImages_32 folder as a fallback. (Default Behavior)

This basically removes unneeded (since we don't do 32-bit testing) noise from the output log.

…ument.

bsudekum · 2018-05-18T20:22:46Z

MapboxNavigation/GenericRouteShield.swift

+import Foundation
+import UIKit
+
+public class GenericRouteShield: StylableView {


Add documentation for this public class.

bsudekum · 2018-05-18T20:23:35Z

MapboxNavigation/GenericRouteShield.swift

+import Foundation
+import UIKit
+
+public class GenericRouteShield: StylableView {


Will this class also suffer from #1392 until a hashKey is exposed?

Good point. We should also be hashing off the foregroundColor of the GenericRouteShield.

bsudekum · 2018-05-18T20:28:28Z

MapboxNavigation/VisualInstructionComponent.swift

            return nil
        }
    }
+
+    var genericCacheKey: String {
+        return "generic-" + (text ?? "nil")


Are there cases we could end up with different shields that have the same cache key, ie generic- because text is nil?

A good test case might be, two different steps where the number is given but no text associated with it? Maybe it can't happen but worth checking out.

@bsudekum The InstructionPresenter doesn't try to render Generic Shields with no text, as they would be an empty box. Even so, this code works because one empty box is equal to another.

bsudekum · 2018-05-18T20:29:22Z

MapboxNavigation/VisualInstructionComponent.swift

            return "\(imageURL.absoluteString)-\(VisualInstructionComponent.scale)"
-        case .delimiter:
+        default:


It might make more sense to keep .delimiter here and remove default so as new cases are added, the compiler will yell at us to handle them.

bsudekum · 2018-05-18T20:30:40Z

MapboxNavigation/DayStyle.swift

+        GenericRouteShield.appearance().backgroundColor = .clear
+        GenericRouteShield.appearance().borderWidth = 1.0
+        GenericRouteShield.appearance().cornerRadius = 5.0
+        GenericRouteShield.appearance().foregroundColor = .black


How does this look for the night style?

Pretty good, all we end up having to do is needing to change the foregroundColor: Line

Oh i missed that, 👍

… applied style. Also doing this to exit components, as it was not quite done correctly.

Adding generic shield support to instruction banner.

fd2a57b

JThramer self-assigned this May 11, 2018

JThramer added feature New feature request. wip labels May 11, 2018

1ec5 reviewed May 11, 2018

View reviewed changes

Jerrad Thramer added 4 commits May 10, 2018 18:52

Improving readability by rewriting delimiter case

58646ea

Updating lockfile in effort to chase down elusive bug

7229115

Fixing issue where public initializer's signature has changed, causin…

e21b226

…g test failures.

updating references in test to new enum

b2b96ca

JThramer force-pushed the jerrad/generic-route-shields branch from 690494f to b2b96ca Compare May 11, 2018 22:03

JThramer force-pushed the jerrad/generic-route-shields branch from f637ffb to b2b96ca Compare May 15, 2018 20:16

Jerrad Thramer added 2 commits May 15, 2018 14:17

Reverting dependancy on MapboxDirections back to 0.20.0

261d2a0

Merge branch 'master' into jerrad/generic-route-shields

76899bf

taking GenericRouteShield public

5a1f541

JThramer force-pushed the jerrad/generic-route-shields branch from 5fba7a6 to 5a1f541 Compare May 16, 2018 18:12

Jerrad Thramer added 6 commits May 16, 2018 15:36

Fixing incorrect logic where we only check for one of the two book-ca…

873a3a2

…sing components are shields

Fixing test failures caused by improper fixtures.

6b19ea3

Updating test to pre-cache the shield image before it requests it

30dabb6

Updating test logic to reflect updated, correct delimiter behavior. E…

3c1795a

…xpecting to see a green light.

Green Light: Fixing issue by ensuring that generic shield components …

218f133

…are always represented by generic cache keys.

JThramer force-pushed the jerrad/generic-route-shields branch from 66a364a to 218f133 Compare May 18, 2018 00:33

Jerrad Thramer added 4 commits May 17, 2018 18:39

Does this really need to be a function? also removing unneeded guard …

cdf29cd

…clause

Adding test to make sure generic instructions are handled properly

273a739

Adding snapshot test for generic shields.

7606f48

updating CHANGELOG.md

d566413

JThramer added ✓ ready for review and removed wip labels May 18, 2018

Jerrad Thramer added 2 commits May 18, 2018 10:54

Adding option in FBSnapshot's verify command to only check "Reference…

45aa460

…Images_64".

Fixing bad test caused by wonky reference image. Also adding the abil…

304d31c

…ity to tune tolerance per-test.

JThramer added the ⏎ Ready To Merge label May 18, 2018

bsudekum reviewed May 18, 2018

View reviewed changes

While we're at it, lets update the other snapshot tests with this arg…

e578eb0

…ument.

bsudekum reviewed May 18, 2018

View reviewed changes

Jerrad Thramer added 3 commits May 18, 2018 16:00

Adding ability for generic shield components to cache off the current…

bdecb49

… applied style. Also doing this to exit components, as it was not quite done correctly.

Removing default case for cacheKey switch logic

a6ebc9e

Adding comments

b7eb8c8

bsudekum approved these changes May 18, 2018

View reviewed changes

JThramer merged commit 6cd26dc into master May 18, 2018

JThramer deleted the jerrad/generic-route-shields branch May 18, 2018 22:31

bsudekum mentioned this pull request Jun 1, 2018

Imageless banner components should include generic shield icon #1190

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding generic shield support to instruction banner. #1417

Adding generic shield support to instruction banner. #1417

JThramer commented May 11, 2018 •

edited

Loading

1ec5 May 11, 2018

1ec5 May 11, 2018

JThramer May 11, 2018 •

edited

Loading

bsudekum May 18, 2018

bsudekum commented May 14, 2018

JThramer commented May 15, 2018

bsudekum May 18, 2018

JThramer May 18, 2018

bsudekum May 18, 2018

bsudekum May 18, 2018

JThramer May 18, 2018

bsudekum May 18, 2018

bsudekum May 18, 2018

JThramer May 18, 2018 •

edited

Loading

bsudekum May 18, 2018

bsudekum May 18, 2018

JThramer May 18, 2018

bsudekum May 18, 2018

Adding generic shield support to instruction banner. #1417

Adding generic shield support to instruction banner. #1417

Conversation

JThramer commented May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented May 14, 2018

JThramer commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer commented May 11, 2018 •

edited

Loading

JThramer May 11, 2018 •

edited

Loading

JThramer May 18, 2018 •

edited

Loading