Fixes #1165: Add Screen Tips #1329

oliviabrown9 · 2018-08-30T21:10:55Z

No description provided.

…to oliviabrown9/screen-tips

oliviabrown9 · 2018-08-30T21:36:01Z

@joeyg Would love if you could take a look at this!

sblatz

Some cleanup necessary here. I encountered the following bugs:

Favorite Site
When using the favorite site tip's cancel button, you then have to press "Done" again, making it cumbersome to get back to the home view.

Siri Shortcut
When we open the Siri Shortcut options, it's not immediately clear where they're at. Is it possible to scroll the tableView to that position before it's loaded so it's easily visible?

No max tips
Currently, every tip is getting shown before the "Browse Erase Repeat" text comes back. On Android we only show 3 tips before going back to that text. Not sure if this is a deliberate change for iOS though!

sblatz · 2018-08-30T23:28:23Z

Blockzilla/AppDelegate.swift

@@ -266,6 +277,10 @@ class AppDelegate: UIResponder, UIApplicationDelegate, ModalDelegate, AppSplashC

 }

+ func applicationWillEnterForeground(_ application: UIApplication) {
+ browserViewController.tipManager = TipManager()


This seems expensive to construct every time we enter the foreground. Is there a smaller function we can run instead?

I personally don't think this is an issue. The only option would be to reset the TipManager's possible tips, but this is essentially all that re-initializing it does as well, unless I'm mistaken.

Fair point! Another side effect: if I background the app and open it again, it's possible I see a tip I just saw (after I navigate away and press erase). This is more related to the "only show 3 tips" thing, but I could hypothetically get 3 of the same tip if I close the app in between each erase, which seems bad IMO

sblatz · 2018-08-30T23:31:30Z

Blockzilla/BrowserViewController.swift

-
- return shouldShowTrackerStatsToUser == true &&
- getNumberOfLifetimeTrackersBlocked(userDefaults: userDefaults) >= 10
+ let shouldShowTipsToUser = userDefaults.object(forKey: BrowserViewController.userDefaultsShareTrackerStatsKeyNEW) as! Bool?


Is this key correct? "userDefaultsShareTrackerStatsKeyNEW"

Unfortunately, yes

sblatz · 2018-08-30T23:35:35Z

Blockzilla/BrowserViewController.swift

+ case TipManager.TipKey.biometricTip, TipManager.TipKey.siriEraseTip:
+ showSettings()
+ case TipManager.TipKey.siriFavoriteTip:
+ if #available(iOS 12.0, *) {


Should this tip be shown at all if the user isn't on iOS 12? Perhaps this if #available should be placed elsewhere?

I construct it so that this tip never gets added to the possible tips in the TipManager. However, because the SiriFavoriteVC is only @/available for iOS 12, the compiler requires an additional check. Users should never have the option to click on this tip (because it will never be possible)

Ah, it's an annoying compiler thing; I was wondering why we were checking twice. Makes sense!

sblatz · 2018-08-30T23:38:28Z

Blockzilla/HomeView.swift

+ tipTitleLabel.textColor = UIConstants.colors.defaultFont
+ tipTitleLabel.font = UIConstants.fonts.shareTrackerStatsLabel
+ tipTitleLabel.numberOfLines = 0
+ tipTitleLabel.minimumScaleFactor = 0.65


nit: magic number (could we change the trackerStatsLabel to use whatever constant this ends up having too?)

sblatz · 2018-08-30T23:40:25Z

Blockzilla/HomeView.swift

@@ -117,21 +162,55 @@ class HomeView: UIView {
 fatalError("init(coder:) has not been implemented")
 }

+ func showTipView() {
+ description1.isHidden = true


I know you didn't add this variable, but this is an awful name 😂 I have no idea what it's referring to. Could you please refactor it if it's not too much work?

sblatz · 2018-08-30T23:48:31Z

Blockzilla/TipManager.swift

+ self.showVc = showVc
+ }
+
+ static func == (lhs: Tip, rhs: Tip) -> Bool {


Love this! 🎉

sblatz · 2018-08-30T23:52:01Z

Blockzilla/TipManager.swift

+ }
+ }
+
+ lazy var autocompleteTip = Tip(title: "Autocomplete URLs for the sites you use most", description: "Long-press any URL in the address bar", identifier: TipKey.autocompleteTip)


I know I said you could hardcode these, so I apologize for the confusion 😅 but can you still put them in UIConstants just not as localizable strings?

sblatz · 2018-08-30T23:52:56Z

Blockzilla/TipManager.swift

+ if canShowTip(with: tip.identifier) {
+ return tip
+ }
+ else {


nit: put else on same line as }

sblatz · 2018-08-30T23:53:12Z

Blockzilla/TipManager.swift

+ else {
+ return fetchTip()
+ }
+


nit: whitespace

sblatz · 2018-08-30T23:53:22Z

Blockzilla/TipManager.swift

+ }
+ return defaults.bool(forKey: id)
+ }
+


nit: whitespace

sblatz

One nit, but looks good to me! 😄

sblatz · 2018-08-31T12:42:58Z

Blockzilla/HomeView.swift

+ }
+ }
+ }
+


nit: whitespace

joeyg

Thanks for your help on this!

joeyg · 2018-08-31T16:49:16Z

Blockzilla/AppDelegate.swift

@@ -50,10 +50,21 @@ class AppDelegate: UIResponder, UIApplicationDelegate, ModalDelegate, AppSplashC
 setupErrorTracking()
 setupTelemetry()
 TPStatsBlocklistChecker.shared.startup()
+ browserViewController.tipManager = TipManager()


Lets flip this around a bit and have TipManager manage the singleton. I think the changes would be:

Add static let shared = TipManager() in the TipManager class

Modify BrowserViewController constructor to include the argument tipmanager: TipManager = TipManager.shared

Modify HomeView constructor to also take in a TipManager and optionally pass that in based on if tips are visible at that time.

joeyg · 2018-08-31T16:50:14Z

Blockzilla/BrowserViewController.swift

- let settingsViewController = SettingsViewController(searchEngineManager: searchEngineManager, whatsNew: browserToolbar.toolset)
-
+ let settingsViewController: UIViewController!
+ if shouldScrollToSiri {


This can be leaned up to just one line:
settingsViewController = SettingsViewController(searchEngineManager: searchEngineManager, whatsNew: browserToolbar.toolset, shouldScrollToSiri: shouldScrollToSiri)

joeyg · 2018-08-31T16:53:16Z

Blockzilla/BrowserViewController.swift

 let settingsNavController = UINavigationController(rootViewController: settingsViewController)
 settingsNavController.modalPresentationStyle = .formSheet

 modalDelegate.presentModal(viewController: settingsNavController, animated: true)

 Telemetry.default.recordEvent(category: TelemetryEventCategory.action, method: TelemetryEventMethod.click, object: TelemetryEventObject.settingsButton)
 }
+
+ @available(iOS 12.0, *)


I feel like there is a lot of @available(iOS 12.0, *) code in here for the Siri tips. Could it be simplified to a static property or function on TipManager? Should make cleanup a bit easier when this is no longer needed.

static func showSiriTips() { ...@avaialble stuff here ?

Cleaned up significantly, though still checking once in TipManager and once in bvc to handle displaying SiriSettings in bvc.

joeyg · 2018-08-31T16:56:23Z

Blockzilla/BrowserViewController.swift

@@ -942,6 +953,7 @@ extension BrowserViewController: URLBarDelegate {
 func urlBarDidLongPress(_ urlBar: URLBar) {
 let customURLItem = PhotonActionSheetItem(title: UIConstants.strings.customURLMenuButton, iconString: "icon_link") { action in
 urlBar.addCustomURL()
+ UserDefaults.standard.set(false, forKey: TipManager.TipKey.autocompleteTip)


I feel like it would be nice to have TipManager take care of setting the UserDefaults value. BrowserViewController shouldn't have to know that TipManager uses UserDefaults

joeyg · 2018-08-31T16:58:03Z

Blockzilla/TipManager.swift

+
+ lazy var requestDesktopTip = Tip(title: UIConstants.strings.requestDesktopTipTitle, description: UIConstants.strings.requestDesktopTipDescription, identifier: TipKey.requestDesktopTip)
+
+ @available(iOS 12.0, *)


Why does the existence of this property need to be hidden to old OS versions?

Blockzilla/AppDelegate.swift

…focus-ios#1329) * Successfully fetching tips * In progress * Add share tracker as a tip * Show description correctly * Support two line description * Deep linking UI * Specific tip strings * Show settings * Handle sessions correctly * Change to 50% * Biometric tip added correctly * Don't show tips after using feature * Whitespace * Remove unnecessary pbxproj changes * Nits * Don't go back to settings * Scroll to siri section * More whitespace * Consisten if else bracing * Fixes mozilla-mobile/focus-ios#1327: Add Telemetry for Screen Tips (mozilla-mobile/focus-ios#1334) * Refactoring

…illa-mobile/focus-ios#1329) * Successfully fetching tips * In progress * Add share tracker as a tip * Show description correctly * Support two line description * Deep linking UI * Specific tip strings * Show settings * Handle sessions correctly * Change to 50% * Biometric tip added correctly * Don't show tips after using feature * Whitespace * Remove unnecessary pbxproj changes * Nits * Don't go back to settings * Scroll to siri section * More whitespace * Consisten if else bracing * Fixes mozilla-mobile/focus-ios#1327: Add Telemetry for Screen Tips (mozilla-mobile/focus-ios#1334) * Refactoring

oliviabrown9 added 15 commits August 28, 2018 15:54

Successfully fetching tips

b307cc3

In progress

8ca6ccf

Merge branch 'v7.0-dev' of https://github.com/mozilla-mobile/focus in…

eae9dff

…to oliviabrown9/screen-tips

Add share tracker as a tip

3d99c20

Show description correctly

4d84a98

Support two line description

299f0cf

Deep linking UI

7ad54c0

Specific tip strings

aa5602e

Show settings

c01b788

Handle sessions correctly

ef82ff9

Change to 50%

a7325f9

Biometric tip added correctly

440baa3

Don't show tips after using feature

9404ff0

Whitespace

1d48dfb

Remove unnecessary pbxproj changes

e9187c4

oliviabrown9 requested a review from sblatz August 30, 2018 21:57

sblatz suggested changes Aug 31, 2018

View reviewed changes

oliviabrown9 added 3 commits August 30, 2018 18:27

Nits

b63404d

Don't go back to settings

bff940e

Scroll to siri section

0fb4f0c

oliviabrown9 requested a review from sblatz August 31, 2018 05:12

oliviabrown9 added 3 commits August 30, 2018 22:15

More whitespace

29b30d7

Consisten if else bracing

0f7a199

Fixes #1327: Add Telemetry for Screen Tips (#1334)

c427898

sblatz approved these changes Aug 31, 2018

View reviewed changes

Blockzilla/HomeView.swift Outdated

}

}

}

Copy link

Contributor

sblatz Aug 31, 2018

Choose a reason for hiding this comment

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

nit: whitespace

joeyg reviewed Aug 31, 2018

View reviewed changes

oliviabrown9 added 2 commits August 31, 2018 14:05

Refactoring

531d420

Merge

14cd89d

oliviabrown9 merged commit 71dbe9f into v7.0-dev Aug 31, 2018

oliviabrown9 deleted the oliviabrown9/screen-tips branch August 31, 2018 21:10

oliviabrown9 mentioned this pull request Aug 31, 2018

Start Screen Tips #1165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1165: Add Screen Tips #1329

Fixes #1165: Add Screen Tips #1329

oliviabrown9 commented Aug 30, 2018

oliviabrown9 commented Aug 30, 2018

sblatz left a comment •

edited

Loading

sblatz Aug 30, 2018

oliviabrown9 Aug 31, 2018

sblatz Aug 31, 2018

sblatz Aug 30, 2018

oliviabrown9 Aug 31, 2018

sblatz Aug 30, 2018

oliviabrown9 Aug 31, 2018

sblatz Aug 31, 2018

sblatz Aug 30, 2018

sblatz Aug 30, 2018

sblatz Aug 30, 2018

sblatz Aug 30, 2018

sblatz Aug 30, 2018

sblatz Aug 30, 2018

sblatz Aug 30, 2018

sblatz left a comment

sblatz Aug 31, 2018

joeyg left a comment

joeyg Aug 31, 2018

joeyg Aug 31, 2018

joeyg Aug 31, 2018

oliviabrown9 Aug 31, 2018

joeyg Aug 31, 2018

joeyg Aug 31, 2018


		lazy var requestDesktopTip = Tip(title: UIConstants.strings.requestDesktopTipTitle, description: UIConstants.strings.requestDesktopTipDescription, identifier: TipKey.requestDesktopTip)

		@available(iOS 12.0, *)

Fixes #1165: Add Screen Tips #1329

Fixes #1165: Add Screen Tips #1329

Conversation

oliviabrown9 commented Aug 30, 2018

oliviabrown9 commented Aug 30, 2018

sblatz left a comment • 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

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

sblatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyg 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

sblatz left a comment •

edited

Loading