[NT-203] (1/2) Pledge summary section #845

Scollaco · 2019-09-17T19:12:16Z

📲 What

Adds the Summary View section to the ManageViewPledgeViewController.

Note:

This PR doesn't contain the final strings. They will be added on the 2/2 part of this work.

JIRA ticket

🤔 Why

This is the first part of the ManagePledge feature. More to come.

🛠 How

Created a new ManagePledgeSummaryView class
Created viewModel to handle the views outputs
Created viewModel tests
Added the new view to ManageViewPledgeViewController

👀 See

Before 🐛	After 🦋

♿️ Accessibility

Supports Dynamic Type

✅ Acceptance criteria

With NativeCheckoutPledgeView feature enabled:

Go to Profile and choose a backed project.
The Backer number should match the real value
The Baking date number should match the real date the user backed the project
The Pledge amount should match the real pledged value
The Shipping cost should match the real shipping cost
If backing has non-shippable reward, no reward or no location available, the Shipping location should be hidden
The Total amount should match the real total amount value

ksr-ci-bot · 2019-09-17T21:11:08Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

Scollaco · 2019-09-17T21:32:57Z

Library/ViewModels/ManagePledgeSummaryViewViewModel.swift

+    let _ = backing.rewardId else {
+    return true
+  }
+  if let reward = backing.reward {


Sometimes a backing can have rewardId but the reward can still be nil

Scollaco · 2019-09-17T21:34:15Z

Library/Format.swift

@@ -144,6 +144,12 @@ public enum Format {
    return NSAttributedString(string: Strings.plus_shipping_cost(shipping_cost: ""), attributes: attributes)
  }

+  public static func attributedAmount(


I created this function to return the attributed amount without the + sign

I can't think of a reason that we'd want to do this 🤔

We can probably remove this function now, right?

justinswart

Cool! Couple of small things.

justinswart · 2019-09-19T21:07:00Z

Kickstarter-iOS/Views/Controllers/ManagePledgeSummaryView.swift

+    |> \.textColor .~ UIColor.black
+    |> \.font .~ UIFont.ksr_subhead().bolded
+    |> \.adjustsFontForContentSizeCategory .~ true
+    |> \.text %~ { _ in "Total amount" }


Best to use localizedString directly until we have the string added.

Actually after merging master I noticed that most of the strings were already there!

justinswart · 2019-09-19T21:13:50Z

Library/ViewModels/ManagePledgeSummaryViewViewModel.swift

+  var outputs: ManagePledgeSummaryViewViewModelOutputs { get }
+}
+
+public class ManagePledgeSummaryViewViewModel: ManagePledgeSummaryViewViewModelType,


I'm not sure when we started calling VM's VVM's 😅. I don't think we should be naming them ViewViewModel, the fact that this VM is used for a View and not a ViewController doesn't matter, we should just call it ManagePledgeSummaryViewModel, I think we should also add an investment day task to fix these names in the codebase for other VVMs.

lol it makes sense. I will rename it

justinswart · 2019-09-19T21:14:08Z

Library/ViewModels/ManagePledgeSummaryViewViewModel.swift

+      .zip(with: backing)
+
+    self.backerNumberText = backing
+      .map { "Backer #\($0.sequence)" }


Please use localizedString until this is translated.

justinswart · 2019-09-19T21:16:56Z

Library/ViewModels/ManagePledgeSummaryViewViewModel.swift

+
+private func formattedPledgeDate(_ backing: Backing) -> String {
+  let formattedDate = Format.date(secondsInUTC: backing.pledgedAt, dateStyle: .long, timeStyle: .none)
+  return "As of \(formattedDate)"


localizedString

justinswart · 2019-09-19T21:43:49Z

Library/Format.swift

@@ -144,6 +144,12 @@ public enum Format {
    return NSAttributedString(string: Strings.plus_shipping_cost(shipping_cost: ""), attributes: attributes)
  }

+  public static func attributedAmount(


I can't think of a reason that we'd want to do this 🤔

justinswart · 2019-09-19T21:44:49Z

Library/ViewModels/ManagePledgeSummaryViewViewModel.swift

+  let combinedAttributes = defaultAttributes
+    .withAllValuesFrom(superscriptAttributes)
+
+  return Format.attributedAmount(attributes: combinedAttributes) + attributedCurrency


Seeing as we don't need the + sign, we can just do:

private func attributedCurrency(with project: Project, amount: Double) -> NSAttributedString? { let defaultAttributes = checkoutCurrencyDefaultAttributes() .withAllValuesFrom([.foregroundColor: UIColor.ksr_green_500]) let superscriptAttributes = checkoutCurrencySuperscriptAttributes() guard let attributedCurrency = Format.attributedCurrency( amount, country: project.country, omitCurrencyCode: project.stats.omitUSCurrencyCode, defaultAttributes: defaultAttributes, superscriptAttributes: superscriptAttributes ) else { return nil } return attributedCurrency }

Much nicer than my solution :)

justinswart · 2019-09-19T21:51:28Z

Library/ViewModels/ManagePledgeSummaryViewViewModel.swift

+      superscriptAttributes: superscriptAttributes
+    ) else { return nil }
+
+  let combinedAttributes = defaultAttributes.merging(superscriptAttributes) { _, new in new }


Using .withAllValuesFrom(superscriptAttributes) is a bit neater and does the same thing 👍

justinswart · 2019-09-19T21:55:52Z

Do we want the date to truncate at these large sizes?

justinswart · 2019-09-19T22:02:19Z

By the way, I noticed there's an autolayout warning when presenting the alert controller, it seems to be a UIKit bug: http://openradar.appspot.com/49289931

justinswart · 2019-09-19T22:10:13Z

I think that we should use the RewardPledgeNavigationController to present this view, that already has the styling applies to its navigation bar.

Scollaco · 2019-09-20T18:32:29Z

Library/ViewModels/ManagePledgeSummaryViewModel.swift

+      .skipNil()
+
+    self.shippingLocationText = backing.ignoreValues()
+      .map { Strings.Shipping() + ": " + "Australia" }


Here Australia is hardcoded because we will use a Backing property called locationName. This is done in another branch (for the 2/2 PR)

Ok, I think we want to add a new string here:

"Shipping: %{country_name}"

Although it seems very much like what you have, by including the : in the string we give the translator the ability to move it around if they need to.

Yeah, before I implemented this, I checked out the Android codebase and that's how it was implemented. I can create a new string on the second part of this work, but I'm not sure if it would make a big difference.
cc @ginarovirosa

hello @Scollaco @justinswart. Indeed having the Shipping: %{country_name} format makes it easier for the translators to work around country name prefixes. Fwiw, it does seem that the Shipping: %{country_name} variable is an old one. We use %{country} now. As you can see in this string for instance: en -> pledges -> checkout_summary -> Shipping_to_country

Thanks @ginarovirosa!

…er/ios-oss into pledge-summary-section

justinswart · 2019-09-24T00:03:28Z

Will wait to hear back from @ginarovirosa on that string.

What about this comment? Is there a max number of lines set? I'm surprised it doesn't wrap on space.

Scollaco · 2019-09-24T14:42:18Z

Will wait to hear back from @ginarovirosa on that string.

What about this comment? Is there a max number of lines set? I'm surprised it doesn't wrap on space.

It didn't have a number of lines set. I had set adjustsFontSizeToFitWidth = true instead. This is the result:

lines = 0	adjustsFontSizeToFitWidth = true

Seeing the difference I'm leaning towards the lines = 0 solution, because then it won't compromise the font size too much depending on the text.

justinswart

Cool, just the string to add in the next PR 👍

Scollaco added 9 commits September 13, 2019 17:12

Added PledgeSummaryView and its styles

fd8b62b

Fixed view styling

de8faea

Added PledgeSummaryViewViewModel

daec623

Alphabetization, viewModel binding

6d93adc

More Styling, Created PledgeSummaryViewViewModelTests class

37e534f

Deleted comments from test class

58bb787

Added tests

5d07ac7

Renamed class to give a better understanding of context, swiftformat

084022b

Added minimum scale factor to labels

b692a75

Scollaco requested a review from justinswart September 17, 2019 19:12

Scollaco assigned justinswart Sep 17, 2019

Fixed logic that hides/shows the shipping location row

09c95ae

Scollaco commented Sep 17, 2019

View reviewed changes

Scollaco changed the base branch from feature-pledge-summary-section to master September 18, 2019 14:48

Scollaco added 3 commits September 18, 2019 10:48

Merge branch 'master' into pledge-summary-section

5bd7bde

Merge branch 'master' into pledge-summary-section

cec5dab

Merge branch 'master' into pledge-summary-section

0a59197

justinswart suggested changes Sep 19, 2019

View reviewed changes

justinswart reviewed Sep 19, 2019

View reviewed changes

Scollaco added 5 commits September 20, 2019 13:27

Renamed viewModel, replaces string literal with localized string

cd45fe1

Added translations, refactor

8097f3c

Replaced UINavigationController with RewardPledgeNavigationController

518137f

Fixed tests

a3e9309

Swiftformat

d6a2aa6

Scollaco commented Sep 20, 2019

View reviewed changes

Scollaco added 5 commits September 20, 2019 16:36

Merge branch 'master' into pledge-summary-section

b6056a8

Deleted unused function

eec03da

Merge branch 'master' into pledge-summary-section

0660471

swiftformat

1826d43

Merge branch 'pledge-summary-section' of https://github.com/kickstart…

2a1ca44

…er/ios-oss into pledge-summary-section

Set number of lines to 0

020f0c8

justinswart approved these changes Sep 24, 2019

View reviewed changes

Merge branch 'master' into pledge-summary-section

91f5995

Scollaco merged commit 360b7b6 into master Sep 24, 2019

Scollaco deleted the pledge-summary-section branch September 24, 2019 18:54

Scollaco changed the title ~~[1/2] Pledge summary section~~ [NT-203] (1/2) Pledge summary section Sep 24, 2019

Scollaco mentioned this pull request Sep 25, 2019

[NT-203] (2/2) Pledge summary section #853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-203] (1/2) Pledge summary section #845

[NT-203] (1/2) Pledge summary section #845

Scollaco commented Sep 17, 2019 •

edited

Loading

ksr-ci-bot commented Sep 17, 2019 •

edited

Loading

Scollaco Sep 17, 2019

Scollaco Sep 17, 2019

justinswart Sep 19, 2019

justinswart Sep 23, 2019

justinswart left a comment

justinswart Sep 19, 2019

Scollaco Sep 20, 2019

justinswart Sep 19, 2019

Scollaco Sep 20, 2019

justinswart Sep 19, 2019

justinswart Sep 19, 2019

justinswart Sep 19, 2019

justinswart Sep 19, 2019

Scollaco Sep 20, 2019

justinswart Sep 19, 2019

justinswart commented Sep 19, 2019

justinswart commented Sep 19, 2019

justinswart commented Sep 19, 2019

Scollaco Sep 20, 2019 •

edited

Loading

justinswart Sep 23, 2019

Scollaco Sep 23, 2019

ginarovirosa Sep 24, 2019 •

edited

Loading

justinswart Sep 24, 2019

justinswart commented Sep 24, 2019 •

edited

Loading

Scollaco commented Sep 24, 2019

justinswart left a comment

[NT-203] (1/2) Pledge summary section #845

[NT-203] (1/2) Pledge summary section #845

Conversation

Scollaco commented Sep 17, 2019 • edited Loading

📲 What

Note:

🤔 Why

🛠 How

👀 See

♿️ Accessibility

✅ Acceptance criteria

With NativeCheckoutPledgeView feature enabled:

ksr-ci-bot commented Sep 17, 2019 • 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

justinswart 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

justinswart commented Sep 19, 2019

justinswart commented Sep 19, 2019

justinswart commented Sep 19, 2019

Scollaco Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ginarovirosa Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart commented Sep 24, 2019 • edited Loading

Scollaco commented Sep 24, 2019

justinswart left a comment

Choose a reason for hiding this comment

Scollaco commented Sep 17, 2019 •

edited

Loading

ksr-ci-bot commented Sep 17, 2019 •

edited

Loading

Scollaco Sep 20, 2019 •

edited

Loading

ginarovirosa Sep 24, 2019 •

edited

Loading

justinswart commented Sep 24, 2019 •

edited

Loading