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

πŸ’²[Native Checkout] RewardCardContainerView snapshots and view model bindings #779

Merged
merged 116 commits into from
Aug 7, 2019

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Aug 1, 2019

πŸ“² What

πŸ€” Why

Rewards can be in many different states based on:

  • whether or not the project is live.
  • if the user has backed the project.
  • if their payment method has errored.
  • the availability of the reward.
  • the position of the 43rd moon of Saturn.

πŸ›  How

I found that it was easier to determine that all the states had been covered by recording snapshot tests with all of the combinations, I will add view model tests in a subsequent PR and link it back here.

πŸ‘€ See

Examples of just some of the states (see 45 recorded snapshots for all of them):

Live, Non-backed, available, timebased Live, backed Live, backed, errored Live, no longer available Live, backed different reward
testLive_NonBackedProject_AvailableTimebasedReward_lang_en_device_phone4_7inch@2x testLive_BackedProject_BackedReward_AvailableTimebasedReward_lang_en_device_phone4_7inch@2x testLive_BackedProject_BackedReward_Errored_AvailableTimebasedReward_lang_en_device_phone4_7inch@2x testLive_NonBackedProject_UnavailableTimebasedReward_lang_en_device_phone4_7inch@2x testLive_BackedProject_NonBackedReward_Errored_NoReward_lang_en_device_phone4_7inch@2x

♿️ Accessibility

  • Tap targets use minimum of 44x44 pts dimensions
  • Works with VoiceOver
  • Supports Dynamic Type

βœ… Acceptance criteria

Given that there are just so many combinations of states (approx 45 if I have it correct), I think looking through the snapshots, the naming of the snapshot tests, carefully inspecting the properties set on the templates and confirming that the state in the snapshot makes sense, it should be fairly sufficient as AC. Of course run it on device and look out for strangeness but I'm not sure what else to suggest for full QA test coverage other than going through each of the combinations.

⏰ TODO

  • Merge in correct styles once that sweep is completed.
  • Add VM tests (separate PR).

dusi and others added 30 commits March 29, 2019 15:38
…tViewController (#647)

* Rename CheckoutViewController to DeprecatedCheckoutViewController

* Rename CheckoutViewModel to DeprecatedCheckoutViewModel

* Remove CheckoutViewModelTests
…ag is enabled (#646)

* Feature-flagging old rewards treatment & tests

* Adding another test

* Fix issue causing test failure

* PR comments and adding other features to the β€œFeature” enum

* Renaming and clearer logic/func naming

* Line break
…VC/VM (#653)

* Rename RewardPledgeViewController.swift to DeprecatedRewardPledgeViewController.swift

* Rename RewardPledgeViewModel.swift to DeprecatedRewardPledgeViewModel.swift

* Rename RewardPledgeViewController to DeprecatedRewardPledgeViewController

* Rename RewardPledgeViewModel to DeprecatedRewardPledgeViewModel

* Update Snapshot tests
* Prepare PledgeViewModel

* Prepare PledgeViewController

* Prepare PledgeDataSource
* Add stepper image resources

* Add convenience function to add subview constrained to edges

* Prepare pledge amount cell

* Implement amount input field

* Pass correct amount and currency values

* Disable selection

* Explicitly declare types in styles

* Add localized strings

* Add iOS 10 support

* Style with explicit types

* Use localized string

* Re-record screenshot tests for the deprecated controller

* Explicitly set accessibility elements

* Extract ascender constrain to private function

* Add background color

* Prevent spacer from collapsing bellow its minimum width

* Fix inputStackViewStyle

* Register cells and footers using their classes for type safety

* Remove unnecessary traitCollectionDidChange handler

* Make style private

* Refactor auto layout helpers
* Back this project buttons setup

* Adding feature flag behaviour

* Tests

* Swiftlint

* Default off

* Revert test changes

* Cleanup

* PR comments and dynamic type support for button

* Updating screenshots to account for accurate button font size

* Wrapping button text and storing reference to shape layer

* Concat constraints

* Renaming button

* Cleanup
* Fix tests

* Extract shared styles

* Add shipping location cell

* Expose only certain elements to VoiceOver

* Add header trait

* Add new line for better readability

* Update tests

* Add arranged subviews to stack view using a functional helper

* Fix font size

* Use point free expression

* Fix tests

* Fix style test
* wip description cell

* wip description cell, new strings

* wip description cell, refactor

* wip- estimated deloivery date from reward

* wip- datasource test

* new snapshots

* stackview refactor

* stackview func name change

* func name change

* corrected datasource test

* refactor

* pr feedback

* pr feedback

* swiftlint fix

* datasource test fix

* datasource test fix

* description string

* fixing lib/framework tests

* new snapshots for deprecated rewards

* using new string

* wip- vm test name change

* pr feedback

* pr feedback

* strings

* snapshot tests

* change padding

* datasource change

* updated strings to correct deprecated pledge view

* new snapshots

* font change, remove unnecessary constraint and view color, alphabetizing

* blended layers and usimng helper function to add arranged subviews to stack view

* πŸ’²[Native Checkout] Reverts strings & snapshots (#671)

* Revert strings

* Update comment

* reverted strings
* New files

* removing scroll view stuff for now

* Tests

* Cleaning up insets

* More cleanup

* Swiftlint

* Adding β€œdeprecated” to more file names

* Removing duplicated file

* Increasing hit area of close button

* PR comments

* PR comments
* PledgeContinueCell

* Common green button styling

* Tests

* Adding a test, cleaning up

* Swiftlint

* Fixing merge conflicts

* PR feedback
* wip description cell

* wip description cell, new strings

* wip description cell, refactor

* wip- estimated deloivery date from reward

* wip- datasource test

* new snapshots

* stackview refactor

* stackview func name change

* func name change

* corrected datasource test

* refactor

* setup delegate

* pr feedback

* pr feedback

* swiftlint fix

* datasource test fix

* datasource test fix

* pledgevm

* description string

* fixing lib/framework tests

* new snapshots for deprecated rewards

* using new string

* wip- presenting trust and safety now

* wip- vm tests

* wip- refactor

* wip

* button

* learn more higlight color change

* snapshot tests

* pr edit

* pr edits

* snapshot tests and strings

* Replace label and button with UITextView for tappable links

* pr feedback

* font size removed
* New files

* removing scroll view stuff for now

* Tests

* Cleaning up insets

* More cleanup

* Swiftlint

* Adding β€œdeprecated” to more file names

* Removing duplicated file

* Increasing hit area of close button

* Hidden scroll view

* Hidden scroll view working

* Refactoring & cleanup

* Use collection view’s content size

* PR updates from comments

* Fix autolayout warnings on device rotation

* Cleanup

* More cleanup

* More PR feedback
* Sheet overlay container VC and animator

* Renaming, fixing rotation issues

* Revert testing code

* Cleanup and fixing a snapshot

* Using keypath lenses

* Updating the comment

* PR comments

* Renaming

* Cleanup

* View helpers

* Style naming

* Fix build

* Remove iOS 11 Availability Checks From Native Checkout (#679)

* Remove iOS 11 availability references

* Fix merge conflict issues

* Renaming suggestions

* Swiftlint
…feature-native-checkout

# Conflicts:
#	Kickstarter-iOS/DataSources/ProjectPamphletContentDataSource.swift
#	Kickstarter-iOS/DataSources/ProjectPamphletContentDataSourceTests.swift
#	Kickstarter-iOS/Views/Controllers/ProjectPamphletViewController.swift
#	Library/ViewModels/DiscoveryFiltersViewModel.swift
#	Library/ViewModels/LiveStreamContainerPageViewModel.swift
#	Library/ViewModels/ProjectPamphletViewModel.swift
…667)

* Fix tests

* Extract shared styles

* Add shipping location cell

* Expose only certain elements to VoiceOver

* Add header trait

* Add new line for better readability

* Add the ability to format currency strings

* Add currency formatter playground

* Add currency formatter tests

* Fix typo

* Remove duplicate type conformance

* Set label background color for better performance

* Move attributed currency string to shared functions

* Disable Swiftlint's line length in tests

* Add convenience function to calculate superscript offset between two UIFonts

* Refactor attributed currency formatter to a NSNumberFormatter subclass

* Expose attributed formatter through Format enum

* Use the new attributed currency formatter

* Move attributes to checkout styles

* Pass shipping information to data source from view model

* Change font size on the input view to match shipping location

* Fix playground

* Fix Swiftlint issue

* Use typealias for string attributes

* Remove dangling playground reference

* Localize string

* Remove obsolete imports
* Navigate to PledgeVC when reward is selected

* One more test

* Renaming

* Updates based on feedback
* Remove SwiftFormat's hoisting rule

* Update SwiftFormat rules

* Format all Swift files added or updated by feature branch

* Fix line length violations

* Re-enable explicit self reference rule

* Format code with explicit self reference ON

* Update .init spacing

* Format decimals

* Format and operator

* Format comments

* Order SwiftFormat rules alphabetically

* Format selectors

* Format ChangeEmailViewController

* Order excluded swiftformat files alphabeticallyr

* Rename UIView+AutoLayout

* Fix formatter causing build errors

* Disable rules on new line

* Reformat indentation

* Rename ksr_swiftformat to format
# Conflicts:
#	KsApi/models/Config.swift
#	bin/format.sh
* Add ios_native_checkout_pledge_view feature flag

* Fix tests

* Fix back button

* Revert "Fix back button"

This reverts commit b2cb23b.

* Add test for featureNativeCheckoutPledgeViewEnabled()

* Only add close button when modal

* Set navigationItem in presenting VC
* Update button styles first pass

* Move color mixing to extension file

* Update Colors.json

* Updated remaining button styles

* Record new snapshots

* Rename playground buttons

* Move button up slightly on empty state VC

* Update empty state snapshots
…eward-cell-state-bindings

# Conflicts:
#	Kickstarter-iOS/Views/RewardCardContainerView.swift
#	Library/Styles/ButtonStyles.swift
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_de_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_de_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_de_device_phone5_8inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_en_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_en_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_en_device_phone5_8inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_es_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_es_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_es_device_phone5_8inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_fr_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_fr_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_fr_device_phone5_8inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_ja_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_ja_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.RewardsCollectionViewControllerTests/testRewards_NonBacker_LiveProject_lang_ja_device_phone5_8inch@2x.png
@justinswart justinswart marked this pull request as ready for review August 2, 2019 22:42
@justinswart justinswart added needs review and removed blocked a PR that is blocked for external reasons labels Aug 2, 2019
@justinswart justinswart changed the base branch from feature-native-checkout to master August 2, 2019 22:46
@justinswart
Copy link
Contributor Author

Reward States

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Looking good!! Couple comments/questions.

@@ -132,7 +132,7 @@ internal final class BetaToolsViewController: UITableViewController {
self.navigationController?.dismiss(animated: true)
}

// MARK: Private Helper Functions
// MARK: - Private Helper Functions
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this just be Functions 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol thanks

@@ -56,8 +56,7 @@ final class FeatureFlagToolsViewController: UITableViewController {
private func updateConfig(with features: Features) {
guard let config = AppEnvironment.current.config else { return }

let updatedConfig = config
|> \.features .~ features
let updatedConfig = config |> \.features .~ features
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ€”

}
}

func testLive_NonBackedProject() {
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to note is that there are actually two different kinds of "live, non-backed project" states - one when the user is logged in, and one when the user is logged out. The difference between these two from the data perspective is the value of project.personalization.isBacking and project.personalization.backing. If the user is logged out, both of these will be nil. If the user is logged in project.personalization.isBacking will be false, and project.personalization.backing will be nil.

It might be worthwhile to add a test for the logged-out state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks will do.

}

private func rewardIsAvailable(project _: Project, reward: Reward) -> Bool {
let limited = !(reward.remaining == nil && reward.endsAt == nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

let isLimited = reward.remaining != nil || reward.endsAt != nil

if let endsAt = reward.endsAt, project.state == .live, endsAt > 0,
endsAt >= AppEnvironment.current.dateType.init().timeIntervalSince1970 {
let (time, unit) = Format.duration(
secondsInUTC: min(endsAt, project.dates.deadline),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would an endsAt timestamp ever be greater than the project deadline? πŸ€”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just how the code was before... didn't really want to change logic in case it was there for some historical reason.

}

private func stateIconImageColor(project: Project, reward: Reward) -> UIColor? {
if userIsBacking(reward: reward, inProject: project) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do a guard instead?

guard userIsBacking(reward: reward, inProject: project) else { return nil }

}

private func stateIconImageName(project: Project, reward: Reward) -> String? {
if userIsBacking(reward: reward, inProject: project) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, a guard is a little cleaner.

* Add 	RewardCardContainerViewModelTests

* Add RewardCardViewModelTests

* Fix test

* Remove shippingAmount from noReward test

* Add logged out non-backer test
Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Lgtm! Very thorough, nice work ⭐️

:shipit:

@justinswart justinswart merged commit a6c9a21 into master Aug 7, 2019
@justinswart justinswart deleted the feature-native-checkout-reward-cell-state-bindings branch August 7, 2019 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants