Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NT-1355] Lights On header bugfix #1224

Merged
merged 4 commits into from
Jun 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions Kickstarter-iOS/Library/SharedFunctionsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -209,4 +209,28 @@ internal final class SharedFunctionsTests: XCTestCase {
func testPledgeAmountSubtractingShippingAmount() {
XCTAssertEqual(ksr_pledgeAmount(700.50, subtractingShippingAmount: 100), 600.50)
}

func testDiscoveryPageBackgroundColor_Control() {
let optimizelyClient = MockOptimizelyClient()
|> \.experiments .~ [
OptimizelyExperiment.Key.nativeProjectCards.rawValue:
OptimizelyExperiment.Variant.control.rawValue
]

withEnvironment(optimizelyClient: optimizelyClient) {
XCTAssertEqual(discoveryPageBackgroundColor(), .white)
}
}

func testDiscoveryPageBackgroundColor_Variant1() {
let optimizelyClient = MockOptimizelyClient()
|> \.experiments .~ [
OptimizelyExperiment.Key.nativeProjectCards.rawValue:
OptimizelyExperiment.Variant.variant1.rawValue
]

withEnvironment(optimizelyClient: optimizelyClient) {
XCTAssertEqual(discoveryPageBackgroundColor(), .ksr_grey_200)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ internal final class ActivitySampleBackingCell: UITableViewCell, ValueCell {

_ = self
|> activitySampleCellStyle
|> \.backgroundColor .~ discoveryPageBackgroundColor()
|> UITableViewCell.lens.accessibilityHint %~ { _ in
Strings.dashboard_tout_accessibility_hint_opens_project()
}
Expand Down
1 change: 1 addition & 0 deletions Kickstarter-iOS/Views/Cells/ActivitySampleFollowCell.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ internal final class ActivitySampleFollowCell: UITableViewCell, ValueCell {

_ = self
|> activitySampleCellStyle
|> \.backgroundColor .~ discoveryPageBackgroundColor()

_ = self.activityStackView
|> activitySampleStackViewStyle
Expand Down
4 changes: 3 additions & 1 deletion Kickstarter-iOS/Views/Cells/DiscoveryEditorialCell.swift
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ final class DiscoveryEditorialCell: UITableViewCell, ValueCell {
|> \.accessibilityTraits .~ [UIAccessibilityTraits.button]
|> \.accessibilityLabel %~ { _ in Strings.Introducing_Lights_On() }
|> \.accessibilityHint %~ { _ in Strings.Support_creative_spaces_and_businesses_affected_by() }
|> \.backgroundColor .~ .clear

_ = self
|> \.backgroundColor .~ discoveryPageBackgroundColor()

_ = self.rootStackView
|> rootStackViewStyle
Expand Down
2 changes: 1 addition & 1 deletion Kickstarter-iOS/Views/Cells/DiscoveryOnboardingCell.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ internal final class DiscoveryOnboardingCell: UITableViewCell, ValueCell {
internal override func bindStyles() {
_ = self
|> baseTableViewCellStyle()
|> \.backgroundColor .~ .clear
|> \.backgroundColor .~ discoveryPageBackgroundColor()
|> DiscoveryOnboardingCell.lens.contentView.layoutMargins %~~ { layoutMargins, cell in
cell.traitCollection.isRegularRegular
? .init(top: Styles.grid(5), left: Styles.grid(30), bottom: 0, right: Styles.grid(30))
Expand Down
1 change: 1 addition & 0 deletions Kickstarter-iOS/Views/Cells/PersonalizationCell.swift
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ final class PersonalizationCell: UITableViewCell, ValueCell {

_ = self
|> baseTableViewCellStyle()
|> \.backgroundColor .~ discoveryPageBackgroundColor()
|> \.accessibilityElements .~ [self.containerView, self.dismissButton]
|> PersonalizationCell.lens.contentView.layoutMargins %~~ { _, cell in
cell.traitCollection.isRegularRegular
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ internal final class DiscoveryPageViewController: UITableViewController {
fileprivate var emptyStatesController: EmptyStatesViewController?
private lazy var headerLabel = { UILabel(frame: .zero) }()
private var onboardingCompletedObserver: Any?
internal var preferredBackgroundColor: UIColor?
private var sessionEndedObserver: Any?
private var sessionStartedObserver: Any?

Expand Down Expand Up @@ -134,10 +133,11 @@ internal final class DiscoveryPageViewController: UITableViewController {
|> \.rowHeight .~ UITableView.automaticDimension
|> \.estimatedRowHeight .~ 200.0

if let preferredBackgroundColor = self.preferredBackgroundColor {
_ = self
|> \.view.backgroundColor .~ preferredBackgroundColor
}
_ = self.view
|> \.backgroundColor .~ (
// Update the background if it is not currently clear (contained in EditorialProjectsViewController)
self.view.backgroundColor != .clear ? discoveryPageBackgroundColor() : self.view.backgroundColor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately snapshot tests wouldn't pass without this - it seems setting the backgroundColor any earlier than bindStyles was ineffective. We only ever set the backgroundColor to .clear when this is contained in EditorialProjectsViewController.

)

_ = self.headerLabel
|> headerLabelStyle
Expand Down Expand Up @@ -311,20 +311,6 @@ internal final class DiscoveryPageViewController: UITableViewController {
self?.present(nav, animated: true, completion: nil)
}

self.viewModel.outputs.backgroundColor
.observeForUI()
.observeValues { [weak self] backgroundColor in
guard let self = self else { return }

_ = self.view
|> \.backgroundColor .~ backgroundColor

_ = self.tableView
|> \.backgroundColor .~ backgroundColor

self.preferredBackgroundColor = backgroundColor
}

self.viewModel.outputs.contentInset
.observeForUI()
.observeValues { [weak self] inset in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ public final class EditorialProjectsViewController: UIViewController {

internal lazy var discoveryPageViewController: DiscoveryPageViewController = {
DiscoveryPageViewController.configuredWith(sort: .distance)
|> \.preferredBackgroundColor .~ .clear
|> \.delegate .~ self
}()

Expand Down Expand Up @@ -100,6 +99,9 @@ public final class EditorialProjectsViewController: UIViewController {
_ = self.view
|> \.backgroundColor .~ UIColor.white

_ = self.discoveryPageViewController.view
|> \.backgroundColor .~ .clear

_ = self.headerView
|> UIView.lens.layoutMargins %~~ { _, view in
view.traitCollection.isRegularRegular
Expand Down Expand Up @@ -181,25 +183,19 @@ public final class EditorialProjectsViewController: UIViewController {
self?.setNeedsStatusBarAppearanceUpdate()
}

self.viewModel.outputs.applyViewTransformsWithYOffset
.observeForControllerAction()
.observeValues { [weak self] y in
self?.applyViewTransforms(withYOffset: y)
}

self.editorialTitleLabel.rac.text = self.viewModel.outputs.titleLabelText
self.closeButton.rac.tintColor = self.viewModel.outputs.closeButtonImageTintColor
}

// MARK: - Layout

private func configureSubviews() {
_ = (self.discoveryPageViewController.view, self.view)
_ = (self.headerView, self.view)
|> ksr_addSubviewToParent()
|> ksr_constrainViewToEdgesInParent()

_ = (self.headerView, self.view)
_ = (self.discoveryPageViewController.view, self.view)
|> ksr_addSubviewToParent()
|> ksr_constrainViewToEdgesInParent()

_ = (self.headerTopLayoutGuide, self.headerView)
|> ksr_addLayoutGuideToView()
Expand Down
14 changes: 8 additions & 6 deletions Library/OptimizelyClientType.swift
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,17 @@ extension OptimizelyClientType {
attributes: optimizelyUserAttributes()
)
}

public func track(eventName: String) {
let userAttributes = optimizelyUserAttributes()
let userId = deviceIdentifier(uuid: UUID())

try? self.track(eventKey: eventName,
userId: userId,
attributes: userAttributes,
eventTags: nil)
try? self.track(
eventKey: eventName,
userId: userId,
attributes: userAttributes,
eventTags: nil
)
}
}

Expand Down Expand Up @@ -131,7 +133,7 @@ public func optimizelyProperties(environment: Environment? = AppEnvironment.curr
"optimizely_experiments": allExperiments
]
}

public func optimizelyUserAttributes(
with project: Project? = nil,
refTag: RefTag? = nil
Expand Down
11 changes: 11 additions & 0 deletions Library/SharedFunctions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,14 @@ public func ksr_pledgeAmount(

return (pledgeAmount as NSDecimalNumber).doubleValue
}

public func discoveryPageBackgroundColor() -> UIColor {
let variant = OptimizelyExperiment.nativeProjectCardsExperimentVariant()

switch variant {
case .variant1:
return UIColor.ksr_grey_200
case .variant2, .control:
return UIColor.white
}
}
15 changes: 10 additions & 5 deletions Library/TestHelpers/MockOptimizelyClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ internal class MockOptimizelyClient: OptimizelyClientType {
var features: [String: Bool] = [:]
var getVariantPathCalled: Bool = false
var userAttributes: [String: Any?]?

// MARK: - Event Tracking Test Properties

var trackedAttributes: [String: Any?]?
var trackedEventKey: String?
var trackedUserId: String?
Expand Down Expand Up @@ -56,9 +56,14 @@ internal class MockOptimizelyClient: OptimizelyClientType {
}

return experimentVariant
}

func track(eventKey: String, userId: String, attributes: [String: Any?]?, eventTags: [String: Any]?) throws {
}

func track(
eventKey: String,
userId: String,
attributes: [String: Any?]?,
eventTags _: [String: Any]?
) throws {
self.trackedEventKey = eventKey
self.trackedAttributes = attributes
self.trackedUserId = userId
Expand Down
2 changes: 1 addition & 1 deletion Library/ViewModels/CategorySelectionViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public final class CategorySelectionViewModel: CategorySelectionViewModelType,
trackOptimizelyClientButtonClicked(buttonTitle: "Skip")
}

self.continueButtonTappedProperty.signal.observeValues { buttonTitle in
self.continueButtonTappedProperty.signal.observeValues { _ in
let optimizelyProps = optimizelyProperties() ?? [:]
AppEnvironment.current.koala.trackOnboardingContinueButtonClicked(optimizelyProperties: optimizelyProps)
trackOptimizelyClientButtonClicked(buttonTitle: "Continue")
Expand Down
10 changes: 4 additions & 6 deletions Library/ViewModels/CategorySelectionViewModelTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -334,11 +334,10 @@ final class CategorySelectionViewModelTests: TestCase {
self.vm.inputs.categorySelected(with: (artIndexPath, .art))
self.vm.inputs.categorySelected(with: (illustrationIndexPath, .illustration))
self.vm.inputs.categorySelected(with: (gamesIndexPath, .games))

XCTAssertNil(self.optimizelyClient.trackedEventKey)
XCTAssertNil(self.optimizelyClient.trackedAttributes)


XCTAssertNil(mockKVStore.onboardingCategories)
XCTAssertFalse(mockKVStore.hasCompletedCategoryPersonalizationFlow)

Expand All @@ -353,7 +352,7 @@ final class CategorySelectionViewModelTests: TestCase {

XCTAssertEqual(encodedCategories, mockKVStore.onboardingCategories)
XCTAssertTrue(mockKVStore.hasCompletedCategoryPersonalizationFlow)

XCTAssertEqual("Continue Button Clicked", self.optimizelyClient.trackedEventKey)
XCTAssertEqual(["Onboarding Continue Button Clicked"], self.trackingClient.events)
XCTAssertEqual(self.trackingClient.properties(forKey: "context_location"), ["onboarding"])
Expand Down Expand Up @@ -428,15 +427,14 @@ final class CategorySelectionViewModelTests: TestCase {
self.scheduler.advance()

self.dismiss.assertDidNotEmitValue()

XCTAssertNil(self.optimizelyClient.trackedEventKey)
XCTAssertNil(self.optimizelyClient.trackedAttributes)


self.vm.inputs.skipButtonTapped()

self.dismiss.assertValueCount(1)

XCTAssertEqual(self.optimizelyClient.trackedEventKey, "Skip Button Clicked")
XCTAssertEqual(self.trackingClient.events, ["Onboarding Skip Button Clicked"])
XCTAssertEqual(self.trackingClient.properties(forKey: "context_location"), ["onboarding"])
Expand Down
20 changes: 2 additions & 18 deletions Library/ViewModels/DiscoveryPageViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,6 @@ public protocol DiscoveryPageViewModelOutputs {
/// Hopefully in the future we can remove this when we can resolve postcard display issues.
var asyncReloadData: Signal<Void, Never> { get }

/// Emits the background color for the view
var backgroundColor: Signal<UIColor, Never> { get }

/// Emits the contentInset for the UITableView
var contentInset: Signal<UIEdgeInsets, Never> { get }

Expand Down Expand Up @@ -229,18 +226,6 @@ public final class DiscoveryPageViewModel: DiscoveryPageViewModelType, Discovery

self.asyncReloadData = self.projectsLoaded.take(first: 1).ignoreValues()

self.backgroundColor = self.viewWillAppearProperty.signal
.map { _ in
let variant = OptimizelyExperiment.nativeProjectCardsExperimentVariant()

switch variant {
case .variant1:
return UIColor.ksr_grey_200
case .variant2, .control:
return UIColor.white
}
}

self.contentInset = self.viewWillAppearProperty.signal
.map { _ in
let variant = OptimizelyExperiment.nativeProjectCardsExperimentVariant()
Expand Down Expand Up @@ -444,9 +429,9 @@ public final class DiscoveryPageViewModel: DiscoveryPageViewModelType, Discovery

let personalizationCellTappedAndRefTag = self.personalizationCellTappedProperty.signal
.mapConst(RefTag.onboarding)

self.personalizationCellTappedProperty.signal
.observeValues { refTag in
.observeValues { _ in
AppEnvironment.current.optimizelyClient?.track(eventName: "Editorial Card Clicked")
}

Expand Down Expand Up @@ -593,7 +578,6 @@ public final class DiscoveryPageViewModel: DiscoveryPageViewModelType, Discovery

public let activitiesForSample: Signal<[Activity], Never>
public let asyncReloadData: Signal<Void, Never>
public let backgroundColor: Signal<UIColor, Never>
public let contentInset: Signal<UIEdgeInsets, Never>
public let dismissPersonalizationCell: Signal<Void, Never>
public let goToActivityProject: Signal<(Project, RefTag), Never>
Expand Down
Loading