Ref #2151 New tab page notifications #2214

iccub · 2020-01-19T23:12:24Z

Summary of Changes

This pull request fixes issue #2151

Submitter Checklist:

Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()

Test Plan:

Screenshots:

Reviewer Checklist:

Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate unit test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable).
Issue is assigned to a milestone (should happen at merge time).

iccub · 2020-01-19T23:14:02Z

BraveRewardsUI/Settings/SettingsViewController.swift

@@ -129,11 +130,13 @@ class SettingsViewController: UIViewController {
  @objc private func rewardsSwitchValueChanged() {
    state.ledger.isEnabled = settingsView.rewardsToggleSection.toggleSwitch.isOn
    updateVisualStateOfSections(animated: true)
+    NotificationCenter.default.post(name: .rewardsToggled, object: nil)


We have to reset NTP notifications when either rewards or ads are toggled

You should use LedgerObserver.rewardsEnabledStateUpdated to manage when rewards status is updated as its status is actually toggled as well in WalletViewController when the user has previous disabled rewards and clicks "Enable rewards" from the panel

iccub · 2020-01-19T23:14:57Z

BraveShared/BraveStrings.swift

+
+// MARK: - New tab page
+extension Strings {
+    public struct NTP {


I experimented a bit here with better strings structure.

iccub · 2020-01-19T23:20:06Z

Client/Frontend/Browser/HomePanel/FavoritesViewController.swift

@@ -161,6 +177,9 @@ class FavoritesViewController: UIViewController, Themeable {
            $0.removeObserver(self, name: .topSitesConversion, object: nil)
            $0.removeObserver(self, name: .privacyModeChanged, object: nil)
        }
+
+        // Navigating away from NTP counts the current notification as showed.
+        Preferences.NewTabPage.brandedImageShowed.value = true


There's one architectural caveat to this solution.
When you have a tab with website open and another empty new tab, navigating from NTP to the first tab counts as tab dismissal and the popup is cleared.

No simple way to prevent that, would require a bigger refactor

bottom sheet vc.

small fix for bottom sheet view.

kylehickinson · 2020-01-20T19:47:02Z

BraveRewardsUI/Localized Strings/Strings.swift

@@ -4,14 +4,14 @@

 import BraveShared

-internal struct Strings {
+public struct Strings {


Reason why we made this internal was because it matches the same symbol as in BraveShared module and caused issues. Not sure if any issues will arise in the future due to this change

I can replace it with extension so there's always one struct

or add duplicate strings, I only took like two small string values from BR-UI

kylehickinson · 2020-01-20T19:49:31Z

BraveRewardsUI/Settings/SettingsViewController.swift

@@ -129,11 +130,13 @@ class SettingsViewController: UIViewController {
  @objc private func rewardsSwitchValueChanged() {
    state.ledger.isEnabled = settingsView.rewardsToggleSection.toggleSwitch.isOn
    updateVisualStateOfSections(animated: true)
+    NotificationCenter.default.post(name: .rewardsToggled, object: nil)


You should use LedgerObserver.rewardsEnabledStateUpdated to manage when rewards status is updated as its status is actually toggled as well in WalletViewController when the user has previous disabled rewards and clicks "Enable rewards" from the panel

kylehickinson · 2020-01-20T19:51:12Z

BraveRewardsUI/Settings/SettingsViewController.swift

  }

  @objc private func adsToggleValueChanged() {
    state.ads.isEnabled = settingsView.adsSection.toggleSwitch.isOn
    updateVisualStateOfSections(animated: true)
+    NotificationCenter.default.post(name: .adsToggled, object: nil)


Same as above, but we don't have an AdsObserver unfortunately. Not sure if you this matters, but ads is also toggled in some states of onboarding (ads region becoming available to user who already opted into rewards):

brave-ios/Client/Frontend/Browser/Onboarding/OnboardingAdsAvailableController.swift

Line 32 in 24c14e7

rewards?.ads.isEnabled = true

onboarding doesn't matter

But I need to observe both ads and rewards state changes. So callback for rewards and notification for ads?

I think that should be good yeah

kylehickinson · 2020-01-20T19:52:43Z

BraveShared/BraveStrings.swift

+extension Strings {
+    public struct NTP {
+        public static let getPaidToSeeThisImage =
+            NSL("ntp.getPaidToSeeThisImage",


Will the script pick up NSL? Can't remember if it needs to actually parse NSLocalizedString

kylehickinson · 2020-01-20T19:54:19Z

Client/Frontend/Browser/BrowserViewController/BVC+Rewards.swift

@@ -150,6 +150,15 @@ extension BrowserViewController {
                }
        }
    }
+
+    // Note: at the moment the toggle value does not matter, the same code is run regardless of its state.
+    @objc func rewardsSettingToggled() {


Why not just have the notification registered to 1 selector then if they are going to run the same code? Also how come we don't care about the value of the actual toggle here?

I can do 1 selector.
We do not care about value because it just resets the NTP notification, and what notification is shown depends on few conditions, rewards state, ads state, image type. Does not need to be shown immediately.

kylehickinson · 2020-01-20T20:12:55Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+
+            func update() {
+                // Update dark blur, the more of content view go away the less dark it is.
+                backgroundOverlayView.alpha = (maxY - yPosition) / maxY


Should probably check maxY != 0 otherwise you can end up with NaN and an exception thrown when you set the frame of contentView below

kylehickinson · 2020-01-20T20:13:55Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+    }
+
+    private var initialDrawerYPosition: CGFloat {
+        let popupY = (view.frame.height / 2) - (contentView.frame.height / 2)


May want to ceil/floor this value, may end up with half pixels

kylehickinson · 2020-01-20T20:14:47Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+        updateDrawerViewConstraints()
+    }
+
+    private func updateDrawerViewConstraints() {


Why not just override updateConstraints and call setNeedsUpdateConstraints() in traitCollectionDidChange ?

kylehickinson · 2020-01-20T20:16:05Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+
+    // MARK: - Animations
+
+    @objc private func didRecognizePanGesture(_ gestureRecognizer: UIPanGestureRecognizer) {


Nit: Name is a bit misleading, it implies it is only called once when it is recognized and not continuously as the user pans.

kylehickinson · 2020-01-20T20:17:59Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+        if gestureRecognizer.state != .ended { return }
+
+        let velocity = gestureRecognizer.velocity(in: contentView).y
+        let landingYPosition = yPosition + velocity / 10


Use projected end point algorithm:

// Distance travelled after decelerating to zero velocity at a constant rate (credit: a WWDC video) func project(initialVelocity: Float, decelerationRate: Float) -> Float { return (initialVelocity / 1000.0) * decelerationRate / (1.0 - decelerationRate) }

Where deceleration rate can be taken from UIScrollView.DecelerationRate.normal

You can see its usage already here:

brave-ios/BraveRewardsUI/Ads/AdsViewController.swift

Lines 174 to 208 in 24c14e7

func project(initialVelocity: CGFloat, decelerationRate: CGFloat) -> CGFloat {

return (initialVelocity / 1000.0) * decelerationRate / (1.0 - decelerationRate)

}

private var panState: CGPoint = .zero

@objc private func dismissPannedAdView(_ pan: UIPanGestureRecognizer) {

guard let adView = pan.view as? AdView else { return }

switch pan.state {

case .began:

panState = adView.center

// Make sure to stop the dismiss timer

dismissTimers[adView]?.invalidate()

case .changed:

adView.transform.ty = min(0, pan.translation(in: adView).y)

case .ended:

let velocity = pan.velocity(in: adView).y

let y = min(panState.y, panState.y + pan.translation(in: adView).y)

let projected = project(initialVelocity: velocity, decelerationRate: UIScrollView.DecelerationRate.normal.rawValue)

if y + projected < 0 {

guard let displayedAd = self.displayedAds[adView] else { return }

hide(adView: adView, velocity: velocity)

displayedAd.handler(displayedAd.ad, .dismissed)

break

}

fallthrough

case .cancelled:

// Re-setup timeout timer

setupTimeoutTimer(for: adView)

adView.layer.springAnimate(property: kPOPLayerTranslationY, key: "translation.y") { animation, _ in

animation.toValue = 0

}

default:

break

}

}

thank you for roasting this class, all this logic was taken from firefox and then adjusted by me

iccub · 2020-01-21T12:23:08Z

BraveShared/BraveStrings.swift

+extension Strings {
+    /// "BAT" or "BAT Points" depending on the region
+    public static var BAT: String {
+      return Preferences.Rewards.isUsingBAP.value == true ? "BAP" : "BAT"


So I duplicated some strings to leave BR-UI strings locked as internal.

Not perfect, we might want to rework that at some point

Yeah this is OK, but in the end we can probably merge them into 1 struct. Reason it didnt start that way was because BraveRewardsUI used to be in its own repo.

kylehickinson

1 more small thing :)

kylehickinson · 2020-01-21T15:10:14Z

Client/Frontend/Widgets/BottomSheetViewController.swift

        let nextYPosition: CGFloat

        let bottomHalfOfChildView = view.frame.height - (contentView.frame.height / 2)
+        let pannedPastHalfOfViewHeight = yPosition > bottomHalfOfChildView
+
+        let closeByVelocity = projectedVelocity > contentView.frame.height


The value of projectedVelocity is distance travelled, not specific endpoint. You'll have to add its current yPosition to this value to compare it against where you want the end point to be (in this case the contentView's maxY)

kylehickinson

UI/Runtime review:

For overall transparent overlay sheet:

The black seems a bit harsh, I get that it helps readability but have we considered using a blurred background instead?

For the claim rewards screen:

Code wise:

Claim my rewards button should execute on touch up instead of touch down

Design wise (assuming there was some design somewhere?)

Can we align the title and close button?
Can we decrease the font-size of the button (and maybe give it some weight), seems so much bigger than the copy

For get paid to turn on ads screen:

Code wise:

Hide sponsored images just immediately sets the pref right? Wasn't supposed to open settings?
Highlights on this button only highlights the image for some reason. It should probably be set up with .system button type so that it highlights everything uniformly

For get paid to turn on rewards:

Learn more action should be executed on touch up, not touch down
Same issue with highlighting as above except there's no image, so there's no feedback at all when you tap either button

For "you can get paid" screen:

Button action should be executed on touch up, not touch down
Same highlight issue

For landscape iPhone:

This overlay takes quite a bit of space, I assume this is fine?

iccub · 2020-01-21T17:13:21Z

The overlay on landscape phone is fine.
We initially planned to replace favorite tiles with the notification view, but there wasn't enough space for small phones.

I will try to add the blur view instead of plain black with alpha

Will fix the code concerns

Rest is just what design team proposed, I will pass this feedback to @jamesmudgett

Add dark blur.

jhreis · 2020-01-21T17:58:00Z

Client/Frontend/Browser/BrowserViewController.swift

+        view.addSubview(vc.view)
+        vc.view.snp.remakeConstraints {
+            $0.right.top.left.equalToSuperview()
+            $0.bottom.equalTo(view)


Why is this separate if . view is the superview?
I think $0.edges.equalToSuperview() should work fine.

jhreis · 2020-01-21T18:00:30Z

Client/Frontend/NTP/NTPNotificationView.swift

+
+class NTPNotificationView: UIStackView {
+
+    private lazy var titleStackView = UIStackView().then {


These shouldn't need to be lazy vars, using let should be fine.

They are, because each of those views don't need to be added to the main view.

Some views have headers and buttons, some don't

jhreis · 2020-01-21T18:04:05Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+            let initialY = initialDrawerYPosition
+
+            // Only move the view if dragged below initial level.
+            if yPosition <= initialY {


This could be reduced to: (but up to you)

// Only move the view if dragged below initial level. yPosition = max(yPosition, initialY) // Dragged all way down, remove the view. yPosition = min(yPosition, maxY)

feels less readable to me, i'll skip

jhreis · 2020-01-21T18:05:26Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+        contentView.snp.remakeConstraints {
+            if showAsPopup || (traitCollection.userInterfaceIdiom == .phone
+            && UIApplication.shared.statusBarOrientation.isLandscape) {
+                $0.bottom.equalToSuperview()


Can combine:
$0.bottom.centerX.equalToSuperview()

jhreis · 2020-01-21T18:06:36Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+        if yPosition == initialDrawerYPosition { return }
+
+        contentView.snp.remakeConstraints {
+            if showAsPopup || (traitCollection.userInterfaceIdiom == .phone


Maybe create const for traitCollection.userInterfaceIdiom == .phone && UIApplication.shared.statusBarOrientation.isLandscape

let iPhoneLandscape = traitCollection.userInterfaceIdiom == .phone && UIApplication.shared.statusBarOrientation.isLandscape if showAsPopup && iPhoneLandscape { }

Probably not needed in this simple situation though.

jhreis · 2020-01-21T18:06:47Z

Client/Frontend/Widgets/BottomSheetViewController.swift

+                $0.width.equalTo(maxHorizontalWidth)
+            } else {
+                $0.leading.trailing.equalToSuperview()
+                $0.bottom.equalToSuperview()


Can add bottom to above line.

Client/Frontend/Widgets/BottomSheetViewController.swift

iccub requested a review from jhreis January 19, 2020 23:12

iccub commented Jan 19, 2020

View reviewed changes

iccub marked this pull request as ready for review January 20, 2020 10:18

iccub added 25 commits January 20, 2020 18:54

Wip: view is working, need polish and pan animation.

75318c8

Swipe down animation working.

5a397d4

Orientation changes should work now.

7156f40

Add iPad support.

dbe2444

Themes support.

27b3c5e

Add translucent view container.

06798f1

Basic helper structure to group all callout views.

ceff5f7

Callout views helper class, move self sizing bottom child vc inside of

05dfc1d

bottom sheet vc.

Primary button helper

64465cd

Add screen: 2 br on, ads on, reg image - claim rewards.

4449220

4 br on, ads on, sponsored- learn more, without links working.

6e5994b

small fix for bottom sheet view.

add link support.

db41a7f

Fix safe area overlay view.

c3de9b5

3 br on, ads on, sponsored

54bb5d8

7 br on, ads off, sponsored

22c4c55

8 br on, ads off, sponsored - learn more

8839476

10 br off, sponsored - learn more

abffb43

Better support for UITextField for landscape and iPad layouts.

eadc648

Basic Branded Image state machine

68d8f3b

Better favorites ntp callout view abstraction, using base class.

a8a14bd

basic learn more flow.

23df1f1

Better bottom inset for one view.

33bbc07

add one more learn handler

6585ab1

wip, better bottom sheet view abstraction

d1f6a82

bottom sheet abstraction finished.

ead2b4a

iccub added 2 commits January 20, 2020 18:54

Making tests to build.

d827372

Show claim rewards notification only for ad grants.

5975a65

iccub force-pushed the feature/ntp_views branch from 92cca40 to 5975a65 Compare January 20, 2020 17:55

kylehickinson suggested changes Jan 20, 2020

View reviewed changes

iccub commented Jan 21, 2020

View reviewed changes

iccub force-pushed the feature/ntp_views branch 4 times, most recently from 44061de to 6249ba1 Compare January 21, 2020 12:31

iccub requested a review from kylehickinson January 21, 2020 12:32

iccub force-pushed the feature/ntp_views branch 3 times, most recently from 49cac92 to c26f53b Compare January 21, 2020 15:06

kylehickinson suggested changes Jan 21, 2020

View reviewed changes

I hope Kyle is happy.

2be02a6

iccub force-pushed the feature/ntp_views branch from c26f53b to 2be02a6 Compare January 21, 2020 15:24

iccub requested a review from kylehickinson January 21, 2020 15:24

kylehickinson suggested changes Jan 21, 2020

View reviewed changes

Fix design problems.

395090b

Add dark blur.

iccub requested a review from kylehickinson January 21, 2020 17:52

Reduce spacing between header and body as per design.

130d24a

jhreis suggested changes Jan 21, 2020

View reviewed changes

Constraints cleanup.

512296a

kylehickinson approved these changes Jan 21, 2020

View reviewed changes

iccub requested a review from jhreis January 22, 2020 17:08

jhreis approved these changes Jan 22, 2020

View reviewed changes

jhreis merged commit 1a4afab into development Jan 22, 2020

jhreis deleted the feature/ntp_views branch January 22, 2020 22:08

jhreis pushed a commit that referenced this pull request Jan 22, 2020

Ref #2151 New tab page notifications (#2214)

4d05c70


		// MARK: - Animations

		@objc private func didRecognizePanGesture(_ gestureRecognizer: UIPanGestureRecognizer) {

	func project(initialVelocity: CGFloat, decelerationRate: CGFloat) -> CGFloat {
	return (initialVelocity / 1000.0) * decelerationRate / (1.0 - decelerationRate)
	}

	private var panState: CGPoint = .zero
	@objc private func dismissPannedAdView(_ pan: UIPanGestureRecognizer) {
	guard let adView = pan.view as? AdView else { return }
	switch pan.state {
	case .began:
	panState = adView.center
	// Make sure to stop the dismiss timer
	dismissTimers[adView]?.invalidate()
	case .changed:
	adView.transform.ty = min(0, pan.translation(in: adView).y)
	case .ended:
	let velocity = pan.velocity(in: adView).y
	let y = min(panState.y, panState.y + pan.translation(in: adView).y)
	let projected = project(initialVelocity: velocity, decelerationRate: UIScrollView.DecelerationRate.normal.rawValue)
	if y + projected < 0 {
	guard let displayedAd = self.displayedAds[adView] else { return }
	hide(adView: adView, velocity: velocity)
	displayedAd.handler(displayedAd.ad, .dismissed)
	break
	}
	fallthrough
	case .cancelled:
	// Re-setup timeout timer
	setupTimeoutTimer(for: adView)
	adView.layer.springAnimate(property: kPOPLayerTranslationY, key: "translation.y") { animation, _ in
	animation.toValue = 0
	}
	default:
	break
	}
	}


		class NTPNotificationView: UIStackView {

		private lazy var titleStackView = UIStackView().then {

Ref #2151 New tab page notifications #2214

Ref #2151 New tab page notifications #2214

Conversation

iccub commented Jan 19, 2020 • edited by jhreis Loading

Summary of Changes

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

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

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

kylehickinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson left a comment

Choose a reason for hiding this comment

iccub commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iccub Jan 21, 2020 • 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

iccub commented Jan 19, 2020 •

edited by jhreis

Loading

iccub Jan 21, 2020 •

edited

Loading