[Feedback][Part 1] Updates `FeedbackType` and introduces `FeedbackSubType` #2419

nishant-karajgikar · 2020-06-24T20:43:32Z

This diff updates the various categories in the FeedbackType enum and introduces infrastructural models ( viz. FeedbackSubType) to build a granular feedback workflow.

1ec5

Since some of these enumerations and such are publicly documented, remember to mention any changes we end up making in the changelog.

Also delete the existing feedback icons from the asset catalog (except for “feedback”, which is for the feedback button).

1ec5 · 2020-06-24T21:53:47Z

MapboxCoreNavigation/Feedback.swift

+    /// Indicates an incorrect visual artifact.
+    case incorrectVisual


I think this category is intended to include things like incorrect visual instructions, not just artifacts. But what we have from the specification is a really weird category name. Not sure if there’s a better thing to call it, at least programmatically where the specification doesn’t care what it’s called.

/cc @JunDai @abhishek1508

I've amended the comment for now. Once this lands on a decision, I'll come back and update this.

1ec5 · 2020-06-24T21:57:36Z

MapboxCoreNavigation/Feedback.swift

+    }
+
+    /// Returns a corresponding list of `FeedbackSubType`s for a given `FeedbackType`
+    public var subtypes: [FeedbackSubType] {


FeedbackSubType is a separate enumeration that contains all the subtypes of all the types, undifferentiated. That means it’s possible for the developer to set a type of confusingAudio but a subtype of routedDownAOneWay, which would be an error on the server side. I think we’ll need either a separate enumeration of subtypes for each type, with each type case having an associated value for its subtype, or roll the whole shebang into a single enumeration, so that turnIconIncorrect would live alongside incorrectVisual in the same enumeration. I’m leaning towards the associated value approach because it enforces better type safety, but it would create some syntactic overhead in any situation where we need to process the feedback type.

Gotcha, i didn't realize that developers would override this subtypes field themselves. But it's a fair point. I'll move over to using the bespoke subtype enums for each top-level category.

1ec5 · 2020-06-24T22:00:07Z

MapboxNavigation/FeedbackItem.swift

+
+public extension FeedbackType {
+
+    // TODO: Localize these strings


To localize these strings, wrap each one in an NSLocalizedString(_:bundle:value:comment:), as with the original strings that are being deleted, then run scripts/extract_localizable.sh to update all the Localizable.strings files.

Will be handling in this in a follow up PR.

Flagged this as action item as part of #2429

1ec5 · 2020-06-24T22:02:17Z

MapboxNavigation/Resources/Assets.xcassets/feedback/confusing_audio.imageset/Contents.json

+  "properties" : {
+    "preserves-vector-representation" : true
+  }


In the asset catalog, change “Render As” to from “Original Image” to “Template Image” so that the icons will adopt the application’s tint color automatically.

Hmm, If i do this the icons seem to disappear. Example:

(I've left Route Preview as default in order to show the difference)

The tint color has always been set to clear, bizarrely enough:

mapbox-navigation-ios/MapboxNavigation/FeedbackViewController.swift

Line 286 in fa19891

cell.imageView.tintColor = .clear

I think we can simply remove that line; the view controller will inherit the application’s tint color.

Hmm, i think these images aren't imported correctly as template images. I see this if I remove that line and make the change to "Render as: template":

Added a note in the description of #2429 to handle this issue.

The problem is that the glyphs within the circles are white instead of transparent. We’ll need to make them transparent so that the template image will show up correctly.

mapbox/navigation-ui-resources#15

#2433 tracks tail work around tint colors.

nishant-karajgikar · 2020-06-25T02:17:35Z

Also delete the existing feedback icons from the asset catalog (except for “feedback”, which is for the feedback button).

Addressed this is in a separate PR #2421

nishant-karajgikar · 2020-06-25T02:26:41Z

@1ec5 , I've made the changes we discussed in #2419 (comment) . Would love a review!

1ec5 · 2020-06-25T16:37:40Z

MapboxCoreNavigation/Feedback.swift

-/// Enum used to define the many `FeedbackSubType`s that may be nested under a given `FeedbackType`
-public enum FeedbackSubType: String {
-    /// Incorrect Visual Subtypes
+/// Enum denoting the subtypes of the  `Incorrect Visual` top-level category


This way jazzy should automatically link the symbol:

Suggested change

/// Enum denoting the subtypes of the `Incorrect Visual` top-level category

/// Enum denoting the subtypes of `FeedbackType.incorrectVisual(subtype:)`.

(Ditto for the other enumerations.)

1ec5 · 2020-06-25T16:38:45Z

MapboxCoreNavigation/Feedback.swift

-    /// Incorrect Visual Subtypes
+/// Enum denoting the subtypes of the  `Incorrect Visual` top-level category
+public enum IncorrectVisualSubtype: String {
+    case none


Instead of an explicit none subtype, make the associated value optional when declaring the FeedbackType case:

case incorrectVisual(subtype: IncorrectVisualSubtype?)

I'd considered this, but the call site looked a little weird ( .incorrectVisual(subtype: nil) for example) to me. But i'm not strongly against this approach.

If it looks too weird, we can make the argument optional with a default value of nil.

1ec5 · 2020-06-25T16:40:15Z

MapboxCoreNavigation/Feedback.swift

-public enum FeedbackSubType: String {
-    /// Incorrect Visual Subtypes
+/// Enum denoting the subtypes of the  `Incorrect Visual` top-level category
+public enum IncorrectVisualSubtype: String {


To avoid cluttering the main namespace, consider moving these enumerations inside of FeedbackType, so that it’s possible to refer to them as e.g. FeedbackType.IncorrectVisualSubtype.

I've made more changes to the FeedbackType enum and the subtype enums in #2429 . It would be easier to do this in that PR. I'll add a note.

Added a note in description of #2429

MapboxCoreNavigation/Feedback.swift

1ec5

These changes look good overall. #2420, #2421, and #2429 continue the work on this feature. #2433 tracks correcting the appearance of the feedback icons.

Remember to update the changelog to note any public API changes.

1ec5 · 2020-07-01T18:07:00Z

MapboxNavigation/FeedbackItem.swift

+    /// Generates a `FeedbackItem` for a given `FeedbackType`
+    /// - Returns: A `FeedbackItem` model object used to render UI
+    func generateFeedbackItem() -> FeedbackItem {
+        return FeedbackItem(title: self.title, image: self.image, feedbackType: self)
+   }


This would be a little cleaner as an initializer on FeedbackItem.

1ec5

Thanks!

1ec5 · 2020-07-02T00:14:16Z

CarPlayManagerTests.testManagerTellsDelegateWhenNavigationStartsAndEndsDueToArrival() seems to be crashing intermittently. Not immediately actionable here but something to watch out for going forward as we upgrade MapboxNavigationNative further.

nishant-karajgikar requested a review from 1ec5 June 24, 2020 20:43

nishant-karajgikar added wip and removed wip labels Jun 24, 2020

nishant-karajgikar mentioned this pull request Jun 24, 2020

[Feedback][Part 2] Remove progress bar from FeedbackViewController #2420

Merged

nishant-karajgikar changed the title ~~Updates FeedbackType and introduces FeedbackSubTypes~~ [Feedback][Part 1]Updates FeedbackType and introduces FeedbackSubTypes Jun 24, 2020

nishant-karajgikar changed the title ~~[Feedback][Part 1]Updates FeedbackType and introduces FeedbackSubTypes~~ [Feedback][Part 1] Updates FeedbackType and introduces FeedbackSubType Jun 24, 2020

nishant-karajgikar self-assigned this Jun 24, 2020

1ec5 reviewed Jun 24, 2020

View reviewed changes

1ec5 reviewed Jun 25, 2020

View reviewed changes

nishant-karajgikar mentioned this pull request Jun 29, 2020

[Feedback][Part 4][WIP] Refactor FeedbackVC and extract TopLevelFeedbackVC + FeedbackDetailsVC #2429

Closed

4 tasks

1ec5 mentioned this pull request Jul 1, 2020

Feedback category icons are white on black regardless of tint color #2433

Closed

3 tasks

1ec5 approved these changes Jul 1, 2020

View reviewed changes

nishant-karajgikar changed the base branch from master to release-v1.0-pre-registry July 1, 2020 20:40

nishant-karajgikar added 6 commits July 1, 2020 19:24

Updates FeedbackTypes and introduces FeedbackSubTypes

d0db616

Update direction of images to handle RTL locales

5c01b13

Use differentiated subtype enums

6611730

Remove case in FeedbackType

b07ea15

Update Changelog

76c2708

Using explicit nil

e4662f7

nishant-karajgikar force-pushed the nishantk/feedbackOverhaul branch from 5f14c24 to e4662f7 Compare July 1, 2020 23:26

1ec5 added this to the v1.0.0 milestone Jul 2, 2020

nishant-karajgikar merged commit 1873c47 into release-v1.0-pre-registry Jul 2, 2020

nishant-karajgikar deleted the nishantk/feedbackOverhaul branch July 2, 2020 17:00

This was referenced Jul 20, 2020

Feedback categories should be localizable #2474

Closed

Add Turkish localization; update translations from Transifex #2475

Merged

1ec5 mentioned this pull request Sep 11, 2020

Add positioning issue category to feedback view controller #2617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feedback][Part 1] Updates `FeedbackType` and introduces `FeedbackSubType` #2419

[Feedback][Part 1] Updates `FeedbackType` and introduces `FeedbackSubType` #2419

nishant-karajgikar commented Jun 24, 2020 •

edited

Loading

1ec5 left a comment

1ec5 Jun 24, 2020

nishant-karajgikar Jun 25, 2020

1ec5 Jun 24, 2020

nishant-karajgikar Jun 25, 2020

1ec5 Jun 24, 2020

nishant-karajgikar Jun 25, 2020

nishant-karajgikar Jun 29, 2020

1ec5 Jun 24, 2020

nishant-karajgikar Jun 25, 2020

1ec5 Jun 25, 2020

nishant-karajgikar Jun 29, 2020

nishant-karajgikar Jun 29, 2020

1ec5 Jun 30, 2020

1ec5 Jun 30, 2020

1ec5 Jul 1, 2020

nishant-karajgikar commented Jun 25, 2020

nishant-karajgikar commented Jun 25, 2020

1ec5 Jun 25, 2020

1ec5 Jun 25, 2020

nishant-karajgikar Jun 25, 2020

1ec5 Jun 30, 2020

nishant-karajgikar Jul 1, 2020

1ec5 Jun 25, 2020

nishant-karajgikar Jun 29, 2020

nishant-karajgikar Jun 29, 2020

1ec5 left a comment •

edited

Loading

1ec5 Jul 1, 2020

1ec5 left a comment

1ec5 commented Jul 2, 2020

		/// Indicates an incorrect visual artifact.
		case incorrectVisual


		public extension FeedbackType {

		// TODO: Localize these strings

	/// Enum denoting the subtypes of the `Incorrect Visual` top-level category
	/// Enum denoting the subtypes of `FeedbackType.incorrectVisual(subtype:)`.

[Feedback][Part 1] Updates FeedbackType and introduces FeedbackSubType #2419

[Feedback][Part 1] Updates FeedbackType and introduces FeedbackSubType #2419

Conversation

nishant-karajgikar commented Jun 24, 2020 • edited Loading

1ec5 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishant-karajgikar commented Jun 25, 2020

nishant-karajgikar commented Jun 25, 2020

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

Choose a reason for hiding this comment

1ec5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

1ec5 commented Jul 2, 2020

[Feedback][Part 1] Updates `FeedbackType` and introduces `FeedbackSubType` #2419

[Feedback][Part 1] Updates `FeedbackType` and introduces `FeedbackSubType` #2419

nishant-karajgikar commented Jun 24, 2020 •

edited

Loading

1ec5 left a comment •

edited

Loading