Order Details: Customer Info #53

mindgraffiti · 2018-05-07T15:43:47Z

Task #11

To Do

Change customer info section to use MVVM

==================================

Display an Order Details view with the customer information section. Note that the Shipping section does not support a phone number property. See API reference.

To Do

Customer information section title
Localize addresses
Localize phone numbers
Validate phone numbers

Shipping

Shipping details title. Display data: customer name, full address

Billing

Billing details title. Display data: customer name, full address
billing phone number, "more" button icon
"more" action sheet: Call, Message
billing email, email button
email button (action): email message presenter
Show/hide billing (button)
Show/hide billing (action)

mindgraffiti · 2018-05-07T15:45:38Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsCustomerInfoCell.swift

+
+extension OrderDetailsCustomerInfoCell {
+    func configure(with viewModel: OrderDetailsViewModel) {
+        title = "hi" // what now? How to tell between shipping or billing display?


So far every viewModel I have implemented has been a 1:1 representation. But the customer info cell gets used for both shipping info and billing info. How do I detect which data to pull from the viewModel?

Hey there @mindgraffiti !!

Since the Order actually has two Addresses, we'd probably need something like AddressViewModel. The Order would have references to the billing / shipping, and you could do...

func configure(with viewModel: AddressViewModel) { }

(Which would hopefully solve the ambiguity issue!)

mindgraffiti · 2018-05-07T15:47:11Z

Hi @jleandroperez, could I bother you with a question about the view model? This PR is not ready for review. It is here for context on the question :)

…section footer title color

houndci-bot · 2018-05-10T14:47:45Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+        let emailButton = UIButton(type: .custom)
+        emailButton.frame = CGRect(x: 8, y: 0, width: 44, height: 44)
+        emailButton.setImage(Gridicon.iconOfType(.mail), for: .normal)
+        emailButton.contentHorizontalAlignment = .right


Natural Content Alignment Violation: Forcing content alignment left or right can affect the Right-to-Left layout. Use naturalContentHorizontalAlignment instead. (natural_content_alignment)

houndci-bot · 2018-05-10T14:47:45Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+        let phoneButton = UIButton(type: .custom)
+        phoneButton.frame = CGRect(x: 8, y: 0, width: 44, height: 44)
+        phoneButton.setImage(Gridicon.iconOfType(.ellipsis), for: .normal)
+        phoneButton.contentHorizontalAlignment = .right


Natural Content Alignment Violation: Forcing content alignment left or right can affect the Right-to-Left layout. Use naturalContentHorizontalAlignment instead. (natural_content_alignment)

houndci-bot · 2018-05-10T14:47:45Z

WooCommerce/Classes/ViewModels/ContactViewModel.swift

+        contact.givenName = address.firstName
+        contact.familyName = address.lastName
+
+        if let organization = address.company  {


Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

mindgraffiti · 2018-05-10T19:52:10Z

Hi @jleandroperez! Would you please review this one? No more questions, it's finally ready!

jleandroperez

Hello there @mindgraffiti !! looks good!!. First review pass.

Let's sync over slack on alternatives to approach UITableViewDataSource!!

jleandroperez · 2018-05-10T20:03:38Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+        let infoNib = UINib(nibName: OrderDetailsCustomerInfoCell.reuseIdentifier, bundle: nil)
+        tableView.register(infoNib, forCellReuseIdentifier: OrderDetailsCustomerInfoCell.reuseIdentifier)
+        let footerNib = UINib(nibName: ShowHideFooterCell.reuseIdentifier, bundle: nil)
+        tableView.register(footerNib, forHeaderFooterViewReuseIdentifier: ShowHideFooterCell.reuseIdentifier)
    }


Small suggestion to reduce code duplication:

let identifiers = [OrderDetailsSummaryCell.reuseIdentifier, OrderDetailsCustomerNoteCell.reuseIdentifier, OrderDetailsCustomerInfoCell.reuseIdentifier, ShowHideFooterCell.reuseIdentifier[ for identifier in identifiers { let nib = UINib(nibName: identifier, bundle: nil) tableView.register(footerNib, forHeaderFooterViewReuseIdentifier: identifier) }

jleandroperez · 2018-05-10T20:07:50Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsCustomerInfoCell.swift

+        }
+        set {
+            titleLabel.text = newValue
+            titleLabel.applyTitleStyle()


applyTitleStyle() sets both, the font and color. Since that doesn't depend on the text, can we move that elsewhere?

@IBOutlet private weak var titleLabel: UILabel! { didSet { titleLabel.applyTitleStyle() } }

jleandroperez · 2018-05-10T20:08:12Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsCustomerInfoCell.swift

+        }
+        set {
+            nameLabel.text = newValue
+            nameLabel.applyBodyStyle()


Same note as above: can we move applyBodyStyle to nameLabel's didSet?

jleandroperez · 2018-05-10T20:08:29Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsCustomerInfoCell.swift

+        }
+        set {
+            addressLabel.text = newValue
+            addressLabel.applyBodyStyle()


Same note as above: can we move applyBodyStyle to the IBOutlet's didSet?

jleandroperez · 2018-05-10T20:10:49Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

            default:
                return UITableViewCell()
        }
    }

+    func tableView(_ tableView: UITableView, heightForHeaderInSection section: Int) -> CGFloat {
+        if sectionTitles[section].isEmpty {
+            return 0.0001


Tip: CGFloat.leastNonzeroMagnitude

Rule of thumb: never ever ever use magic numbers (those are "constants inline"). You'd usually want to define constants for those.

Thanks! I knew .zero but didn't know .leastNonzeroMagnitude existed

jleandroperez · 2018-05-10T20:25:31Z

WooCommerce/Classes/ViewModels/ContactViewModel.swift

+    }
+}
+
+extension CNContact {


Can we please move this one to it's own file? (for clarity mostly!)

jleandroperez · 2018-05-10T20:29:22Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+            return cell
+        }
+        let zeroFrame = CGRect(x: 0, y: 0, width: 0, height: 0.001)
+        return UIView(frame: zeroFrame)


(Almost sure!!) you can just do return nil here. Did that trigger a bug on your end?

If so... a cute way to do this is with just return UIView(frame: .zero)

Ok, so the bug is in -heightForFooterSection, where it has to return a tiny footer height in order to collapse empty section footers. Thanks for the reminder, this one should return nil, not the tiny UIView.

jleandroperez · 2018-05-10T20:31:08Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+    }
+
+    func tableView(_ tableView: UITableView, viewForFooterInSection section: Int) -> UIView? {
+        if section == Section.info.rawValue {


Tip tip:

You'd normally wanna avoid code indentation (Since it's harder to read / grasp what belongs to what brace).

Since there's a single case here, you could do:

guard section == Section.info.rawValue else { return nil }

(And the biggest block gets one less tab)

jleandroperez · 2018-05-10T20:31:34Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+        phoneButton.tintColor = StyleManager.wooCommerceBrandColor
+        phoneButton.addTarget(self, action: #selector(phoneButtonAction), for: .touchUpInside)
+
+        let iconView = UIView(frame: CGRect(x: 0, y: 0, width: 44, height: 44))


Can we please please move all of the magic numbers to constants? (view frames + reuse identifiers)

jleandroperez · 2018-05-10T20:31:43Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+        emailButton.tintColor = StyleManager.wooCommerceBrandColor
+        emailButton.addTarget(self, action: #selector(emailButtonAction), for: .touchUpInside)
+
+        let iconView = UIView(frame: CGRect(x: 0, y: 0, width: 44, height: 44))


(Same comment as above)

… functions to more swift-like

houndci-bot · 2018-05-11T15:47:55Z

WooCommerce/Classes/ViewModels/ContactViewModel.swift

+        }
+    }
+}
+


Trailing Newline Violation: Files should have a single trailing newline. (trailing_newline)

mindgraffiti · 2018-05-11T15:50:44Z

@jleandroperez thank you for making this code look cute! Ready for another look.

jleandroperez

Few nipticky comments!!

jleandroperez · 2018-05-11T15:59:12Z

WooCommerce/Classes/ViewModels/ContactViewModel.swift

+    var formattedAddress: String
+    var cleanedPhoneNumber: String?
+    var phoneNumber: String?
+    var email: String?


Please please: Can we switch everything to let ?

jleandroperez · 2018-05-11T15:59:30Z

WooCommerce/Classes/ViewModels/ContactViewModel.swift

+        cleanedPhoneNumber = address.phone?.components(separatedBy: CharacterSet.decimalDigits.inverted).joined()
+
+        let cnAddress = contact.postalAddresses.first
+        let postalAddress = cnAddress!.value


Can we use a safe unwrap here, just in case?

changed formattedAddress to an optional

jleandroperez · 2018-05-11T16:06:41Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+            self?.setShowHideFooter()
+            // it would be nice to have `tableView.reloadSections([section], with: .bottom)`
+            // but iOS 11 has an ugly animation bug https://forums.developer.apple.com/thread/86703
+            tableView.reloadData()


We've got this working in WPiOS:

1. Log into a wpcom account (in wpios) 2. Open Me > Notification Settings 3. Press over 'View all...'

(That's the animation you'd like to do here!) Snippet here

oh thank you! This looks so much better now.

jleandroperez · 2018-05-11T16:08:21Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+            controller.messageComposeDelegate = self
+            present(controller, animated: true, completion: nil)
+        }
+    }


We should definitely split this one in two. Reasoning is sendTextMessage doesn't imply the method may or may not do something.

You'd normally cut this this way:

func sendTextMessageIfPossible() { } private func sendTextMessage() {}

Good point, I didn't consider the implications. Glad to make it more explicit

jleandroperez · 2018-05-11T16:08:30Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+            controller.mailComposeDelegate = self
+            present(controller, animated: true, completion: nil)
+        }
+    }


(Same as above!)

jleandroperez

Beautiful!!! 🎉

jleandroperez · 2018-05-11T17:24:45Z

WooCommerce/Classes/ViewRelated/Orders/OrderDetails/OrderDetailsViewController.swift

+        phoneButton.addTarget(self, action: #selector(phoneButtonAction), for: .touchUpInside)
+
+        let iconView = UIView(frame: Constants.accessoryFrame)
+        iconView .addSubview(phoneButton)


Nipticky: extra space

mindgraffiti added 4 commits May 7, 2018 10:23

Start implementing MVVM for Customer Information section

96c6ecb

Add footer section color

7636362

Update to CocoaPods 1.5.0

d580ba9

Per Orders i4, name the filter "Custom"

3ee9102

mindgraffiti added type: question Primarily a question about how code works, etc. Size: L labels May 7, 2018

mindgraffiti self-assigned this May 7, 2018

mindgraffiti added this to Icebox in MVLP Kanban Board via automation May 7, 2018

mindgraffiti commented May 7, 2018

View reviewed changes

mindgraffiti added the [Status] Not Ready for Review label May 7, 2018

mindgraffiti requested a review from jleandroperez May 7, 2018 15:46

mindgraffiti added 4 commits May 8, 2018 14:11

Initial AddressViewModel

c05e027

Change AddressViewModel to ContactViewModel

c0ad5b6

Access items in an array safely

dae606a

Force-unwrap address

50911f0

mindgraffiti moved this from Icebox to In Progress in MVLP Kanban Board May 9, 2018

astralbodies moved this from In Progress to Review/Testing in MVLP Kanban Board May 9, 2018

mindgraffiti added 6 commits May 9, 2018 13:58

Decode phone and email address if present

dfc516b

Create a static function instead of an initializer

d805506

Add PhoneNumberKit CocoaPod

57656c0

Reconnect Customer Info shipping and billing details

a38ea0c

Update show/hide footer, add section header / footer heights, adjust …

4626cfb

…section footer title color

Better spacing for show/hide section footer

9ceb597

mindgraffiti removed the type: question Primarily a question about how code works, etc. label May 9, 2018

Merge branch 'develop' into issue/11-customer-info-mvvm

945ab2a

houndci-bot reviewed May 10, 2018

View reviewed changes

mindgraffiti added 3 commits May 10, 2018 09:49

Update podfile.lock

8c78081

Fix merge conflicts

79057c4

Phone Call / Message actions and Email action added

c5f630d

mindgraffiti removed the [Status] Not Ready for Review label May 10, 2018

Fix hound violations

09f50f3

jleandroperez requested changes May 10, 2018

View reviewed changes

No magic numbers, move label styling to didSet or awakeFromNib, obj-c…

ff43486

… functions to more swift-like

houndci-bot reviewed May 11, 2018

View reviewed changes

Fix Hound violation

c7b60ce

jleandroperez requested changes May 11, 2018

View reviewed changes

mindgraffiti added 6 commits May 11, 2018 12:19

Code review changes

0cc8eb3

Remove unused CocoaPod PhoneNumberKit

a0fc615

Convert formattedAddress to an optional

d0db6a6

🔥

d211ef3

Accidentally committed var instead of let. Oops.

b719146

Remove a couple of unnecessary constraints

22fc002

jleandroperez approved these changes May 11, 2018

View reviewed changes

mindgraffiti merged commit df273f2 into develop May 11, 2018

MVLP Kanban Board automation moved this from Review/Testing to Done/Merged May 11, 2018

mindgraffiti deleted the issue/11-customer-info-mvvm branch May 11, 2018 20:15

astralbodies mentioned this pull request Jun 27, 2018

Order Details #21

Closed

9 tasks

jleandroperez added this to the External closed beta milestone Jun 27, 2018

jleandroperez changed the title ~~Issue/11 customer info mvvm~~ Order Details: Customer Info Jun 27, 2018

Order Details: Customer Info #53

Order Details: Customer Info #53

Conversation

mindgraffiti commented May 7, 2018 • edited

Task #11

To Do

To Do

Shipping

Billing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mindgraffiti commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mindgraffiti commented May 10, 2018

jleandroperez 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

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

mindgraffiti commented May 11, 2018

jleandroperez 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

jleandroperez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mindgraffiti commented May 7, 2018 •

edited