PI-1738: Billing Form component: Standard #144

ehab-al-cko · 2022-04-26T20:20:16Z

URL:
https://checkout.atlassian.net/browse/PIMOB-956

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce to Frames Ios?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
[x ] New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

harry-brown-cko · 2022-04-26T20:27:51Z

Base branch should be 4.0.0_RC

harry-brown-cko · 2022-04-27T11:58:17Z

Few notes on UI and behaviour:

prefilled address is not populated
scrolling form is not working when dragging from text fields (dismisses view instead)
header is currently part of the scroll view and can be scrolled off of the screen. should the header be static?
phone number clears after scrolling off of view (unable to click done as result)

harry-brown-cko

approach and testing looks good. few comments, mainly typos. a few quirks with behaviour as well, see #144 (comment)

.../Style/Default Implmentation/BillingForm/Fields/Postcode/DefaultFormStylePostcodedCell.swift

Source/UI/NewUI/BillingForm/Style/Cell/BillingFormTextFieldCellStyle.swift

...Default Implmentation/BillingForm/Fields/AddressLine1/DefaultFormStyleAddressLine1Cell.swift

Source/UI/NewUI/BillingForm/Style/Protocols/FormButtonStyle.swift

...m/Style/Default Implmentation/BillingForm/Fields/Fullname/DefaultFormStyleFullNameCell.swift

Source/UI/Controllers/CardViewController.swift

Source/UI/NewUI/BillingForm/View/Field/BillFormTextField.swift

Source/UI/NewUI/BillingForm/ViewModel/BillingFormCellType.swift

melting-snowman

A few opinions and some feedback after a limited, initial review. In my opinion there is more to cover but I am also not familiar with the standards expected so don't want my opinion to get in the way too much either.

Source/UI/Controllers/CardViewController.swift

Source/UI/NewUI/BillingForm/ViewController/BillingFormViewController.swift

Source/UI/NewUI/BillingForm/ViewModel/BillingFormCellType.swift

Source/UI/NewUI/BillingForm/ViewModel/DefaultBillingFormViewModel.swift

iOS Example Frame SPM/iOS Example Frame/MainViewController.swift

melting-snowman · 2022-04-27T13:06:17Z

Source/UI/NewUI/BillingForm/Factory/BillingFormFactory.swift

+
+struct BillingFormFactory {
+    // in order
+    static let styles: [BillingFormTextFieldCellStyle] = [


Could these styles just be an enum case?

Lets imagine:

enum BillingFormTextFieldCellStyle { case fullName case addressLine1 ..... func getConfiguration() -> BilingFormTextFieldConfig { switch self { case .fullName: return .init(color1: .... , color2: ...) } } }

My initial thought is it would reduce a lot of the syntactic sugar you currently have and may increase configuration options as well as extensibility. Additionally, you could completely drop this styles property if you make the enum declaration conform to CaseIterable and end up with a syntax of .allCases at call site instead of the current styles reference

Sorry, before resolving can we discuss on it? Or at least open a discussion forum somewhere (Git, Slack, anything), so we can try and not forget about it.

@ehab-al-cko to Create Tech investment ticket to address this in new PR

Source/UI/NewUI/BillingForm/Factory/BillingFormFactory.swift

melting-snowman · 2022-04-27T13:10:03Z

Source/UI/NewUI/BillingForm/Style/Cell/BillingFormHeaderCellStyle.swift

+import Foundation
+
+// Header Cell
+public protocol BillingFormHeaderCellStyle {


Not certain if the comment above the protocol provides any value? The type naming should clearly convey it, and if we want the user of the syntax to flag some helper comment, we should at least use the description marker (///)

Not going to lie, going ahead with these comments above the class/protocol declaration makes our publicly exposed code look unfinished.

It would be nice if our exposed code would make CKO appear knowledgeable and professional, not showing useless comments. This occurs in a few places, happy to flag them if we decide to do something about it.

melting-snowman

As it stands I think the lack of cell dequeuing is something that absolutely must be documented if intended or fixed.

UITableView. dequeueReusableCell

For performance reasons, a table view’s data source should generally reuse UITableViewCell objects when it assigns cells to rows in its tableView(_:cellForRowAt:) method

...Default Implmentation/BillingForm/Fields/AddressLine1/DefaultFormStyleAddressLine1Cell.swift

Source/UI/NewUI/BillingForm/View/Field/BillingFormTextFieldView.swift

Tests/Mocks/BillingFormTextFieldCellMockDelegate.swift

Source/Validator/StateValidator.swift

Tests/Mocks/AddressViewControllerMockDelegate.swift

Tests/UI/New UI/BillingForm/Validator/AddressLine2ValidatorTests.swift

Tests/UI/New UI/BillingForm/Validator/PostCodeValidatorTests.swift

Source/UI/NewUI/BillingForm/ViewModel/DefaultBillingFormViewModel.swift

Source/Extensions/UIColor+Extension.swift

Source/UI/Controllers/CardViewController.swift

...rm/Style/Default Implementation/BillingForm/Header/Cancel/DefaultCancelButtonFormStyle.swift

Source/UI/NewUI/BillingForm/View/Field/BillingFormPhoneNumberText.swift

Source/UI/NewUI/BillingForm/View/Field/BillingFormTextField.swift

Source/UI/NewUI/BillingForm/View/Field/BillingFormTextFieldView.swift

melting-snowman · 2022-04-28T16:17:17Z

Source/UI/NewUI/BillingForm/View/TableViewCell/Header/BillingFormHeaderCell.swift

+        return view
+    }()
+
+    private lazy var headerLabel: UILabel = {


I can't see this field used outside of the creation and setup, so I wonder if we need to have this property, or when we create it for the view, we just create, customise, add to view and allow to lose local reference.

Probably other similar UI components are doing the same thing?

Resolved on slack

deepesh-vasthimal-cko · 2022-05-06T20:25:59Z

Trying to run this branch locally to review the UI, I see some build issue. Please can you check below and suggest.

ehab-al-cko · 2022-05-10T08:52:41Z

Trying to run this branch locally to review the UI, I see some build issue. Please can you check below and suggest.

Fixed

Source/UI/NewUI/BillingForm/View/Field/BillingFormPhoneNumberText.swift

melting-snowman · 2022-05-10T10:32:31Z

Cartfile.resolved

@@ -1 +1 @@
-github "marmelroy/PhoneNumberKit" "3.3.3"
+github "marmelroy/PhoneNumberKit" "3.3.7"


I know this review dragged longer than it was expected, but for visibility 3.4.0 was released just few hours ago 🙈 . It has added encoding and decoding strategies, so if that is something of interest we may create some separate maintenance task to review that functionality.

-> Revert to 3.3.3

Should be reverted to 3.3.3

melting-snowman · 2022-05-10T10:34:39Z

Source/CheckoutColor.swift

+}
+
+extension UIColor {
+   fileprivate convenience init(hex: String, alpha: CGFloat = 1.0) {


Might be nice to have this internal and unit tested with valid colour, invalid string, as well as with / without # .

melting-snowman · 2022-05-10T10:44:45Z

Source/Extensions/StringExtensions.swift

+        let bundle = getBundle(forClass: forClass)
+        guard let urlPath = bundle.url(forResource: self, withExtension: "pdf") else { return nil }
+        guard let document = CGPDFDocument(urlPath as CFURL) else { return nil }
+        guard let page = document.page(at: 1) else { return nil }


~~I would document this page 1, as the method currently is slightly unclear. I'd expect that given a string, it will convert a pdf file from resources to an image.~~

~~Technically, we are converting one page at a time, and if I was to reuse this for an actual PDF document, not just an image file it would deliver wrong expectation~~

As I was documenting this comment, I realised using assets in SPM is more what you need and all of this could be avoided to begin with?

TODO: a new ticket to refactor and test this change

melting-snowman · 2022-05-10T10:46:22Z

Source/Resources/en.lproj/Localizable.strings

+"billingAddressTitle" = "Billing address";
+"city" = "City";
+"cancel" = "Cancel";
+"done" = "Done";


Do we have a tracking option to ensure the localisation is being followed after this PR is merged?

This will be solved by @patrick-hoban-cko and @deepesh-vasthimal-cko

Source/UI/Controllers/CardViewController.swift

melting-snowman · 2022-05-10T10:54:17Z

Source/UI/Controllers/CardViewController.swift

+            navigationController?.pushViewController(addressViewController, animated: true)
+            return
+        }
+        navigationController?.present(viewController, animated: true)


Outside of the scope of this PR, but @harry-brown-cko , @deepesh-vasthimal-cko , I think the animated value should follow the decisions of the user of the SDK.

I would say we should offer it as a customisable option, that defaults from user device UIAccessibility.isReduceMotionEnabled. May be worth checking if maybe OS overrides anyway, but there are many very valid reasons for animations to be disabled for an app (mostly accessibility but not only), so the SDK consumer may appreciate us following their approach or even improving it.

Feedback from @patrick-hoban-cko

melting-snowman · 2022-05-10T10:55:20Z

Source/UI/NewUI/BillingForm/Factory/BillingFormFactory.swift

+
+struct BillingFormFactory {
+    // in order
+    static let styles: [BillingFormTextFieldCellStyle] = [


Sorry, before resolving can we discuss on it? Or at least open a discussion forum somewhere (Git, Slack, anything), so we can try and not forget about it.

melting-snowman · 2022-05-10T11:00:24Z

Source/UI/NewUI/BillingForm/Style/Cell/BillingFormHeaderCellStyle.swift

+import Foundation
+
+// Header Cell
+public protocol BillingFormHeaderCellStyle {


Not going to lie, going ahead with these comments above the class/protocol declaration makes our publicly exposed code look unfinished.

It would be nice if our exposed code would make CKO appear knowledgeable and professional, not showing useless comments. This occurs in a few places, happy to flag them if we decide to do something about it.

melting-snowman · 2022-05-10T11:07:30Z

Source/UI/NewUI/BillingForm/Style/Protocols/InputLabelStyle.swift

+import UIKit
+
+// Label element
+public protocol InputLabelStyle {


Before resolving, would you mind providing some form of closure to my argument? I would even settle for a follow up somewhere else.

All of this code is public facing, and I'd like to hope many dev teams would use it. I would like to know that when the likes of NewDay or other fintechs with large iOS teams dig into our exposed code, they will be excited and trust us. Not make us look like we have a weak grasp of Swift / iOS functionality and question the dependency in the first place.

melting-snowman

Tried running locally. Not sure what state the functionality progress is in, but I got this.

Some opinions on this screenshot alone:

the * in the label name is not matching the expectation of being compulsory or not
Country appears twice, once compulsory, once with lower case letter
It is not possible to dismiss keyboard by tapping background
screen is dismissible by simply scrolling down the list too far by mistake

Additionally, there is a memory leak. In screenshot you can see Billing not presented, but 6 VCs in memory and a number of other entries.

…oller`

ehab-al-cko · 2022-05-10T12:09:22Z

Tried running locally. Not sure what state the functionality progress is in, but I got this.

Some opinions on this screenshot alone:

the * in the label name is not matching the expectation of being compulsory or not

This is localised string, I will ask Patrick for it

Country appears twice, once compulsory, once with lower case letter

Country is not implemented Yet, Next ticket is for Country logic

It is not possible to dismiss keyboard by tapping background

This is can be done with next ticket

screen is dismissible by simply scrolling down the list too far by mistake

Need more details for it.

I will test it

Additionally, there is a memory leak. In screenshot you can see Billing not presented, but 6 VCs in memory and a number of other entries.

deepesh-vasthimal-cko

Approved but do not me merge as the base branch needs to be origin/release/4.0.0_RC

…be filled

ehab-al-cko force-pushed the PI-1738_billing_form_component branch from 1d730c6 to 5d6ee39 Compare April 26, 2022 20:34

harry-brown-cko reviewed Apr 27, 2022

View reviewed changes

melting-snowman reviewed Apr 27, 2022

View reviewed changes

melting-snowman suggested changes Apr 27, 2022

View reviewed changes

melting-snowman reviewed Apr 28, 2022

View reviewed changes

Source/UI/NewUI/BillingForm/ViewModel/DefaultBillingFormViewModel.swift Outdated Show resolved Hide resolved

melting-snowman reviewed Apr 28, 2022

View reviewed changes

deepesh-vasthimal-cko reviewed May 10, 2022

View reviewed changes

Source/UI/NewUI/BillingForm/View/Field/BillingFormPhoneNumberText.swift Outdated Show resolved Hide resolved

ehab-al-cko force-pushed the PI-1738_billing_form_component branch from a425692 to f8036a8 Compare May 10, 2022 09:49

ehab-al-cko added 18 commits May 10, 2022 11:34

PI-1738: Billing form component: standard

a3ddaae

Fix logging issue

1d5aae9

Fix DefaultFormStylePostcodedCell typo

02641fb

Fix 'hight' typo

8bb68d2

Fix typo in folder name

0201e01

remove explicite Bool

0f918de

Fix 'BillForm' to be BillingForm

4373e90

Remove unused commented code

e2efe8f

fix deleagte typo

86fed89

Renaming some variables doneButton and cancelButton

77e1998

Update UIColor+Extension

3551409

remove unused code

311a63a

Renaming correction

7a61071

Following the same code style pattern, remove extra self

88bc829

Simplifying the code

d0f46cf

Remove unused import UIKit

f94e42a

Use XCTAssertFalse or XCTAssertTrue instead of XCTAssertEqual

5b742d9

Clean code

9e5b7a2

ehab-al-cko added 6 commits May 10, 2022 11:34

Update checkoutEventLoggerKitVersion to 1.2.0

3fa1922

Change textfield color to red when it has error

db069fa

Fix screen leading and trailing

d1df920

Update country code

ecea7cd

Expose DefaultBillingFormViewModel delegate

b366298

Rename setupViews to setupViewsInOrder

0634474

ehab-al-cko force-pushed the PI-1738_billing_form_component branch from f8036a8 to 0634474 Compare May 10, 2022 10:35

Clean test cases

dbd2ef5

melting-snowman reviewed May 10, 2022

View reviewed changes

clean code

dd4a5b2

melting-snowman reviewed May 10, 2022

View reviewed changes

Join the guard statements for isNewUI and `getBillingFormViewContr…

0c01976

…oller`

ehab-al-cko added 7 commits May 10, 2022 14:09

Fix Memory Leak

5e74377

TODO: CountryCode

61b42c0

//TODO: migrate to assets

3b53468

revert to 3.3.3

07a18fd

Revert PhoneNumberKit to 3.3.0

bad5997

Add required Fonts

1dedcc8

Update padding for header view

a57c2d7

deepesh-vasthimal-cko approved these changes May 12, 2022

View reviewed changes

deepesh-vasthimal-cko added the Dont merge dont merge PR label May 12, 2022

Clean code

b538fde

ehab-al-cko force-pushed the PI-1738_billing_form_component branch from ccef6cb to b538fde Compare May 13, 2022 09:09

ehab-al-cko added 2 commits May 13, 2022 10:40

Fix Carthage build error

abd3e24

Add: A merchant can choose if that field is optional or mandatory to …

bbcdcba

…be filled

ehab-al-cko closed this May 13, 2022

ehab-al-cko deleted the PI-1738_billing_form_component branch May 13, 2022 18:02

ehab-al-cko removed the Dont merge dont merge PR label Jun 9, 2022

ehab-al-cko mentioned this pull request Jun 9, 2022

Frames SDK API specs comments #166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PI-1738: Billing Form component: Standard #144

PI-1738: Billing Form component: Standard #144

ehab-al-cko commented Apr 26, 2022 •

edited

Loading

harry-brown-cko commented Apr 26, 2022

harry-brown-cko commented Apr 27, 2022 •

edited

Loading

harry-brown-cko left a comment •

edited

Loading

melting-snowman left a comment

melting-snowman Apr 27, 2022

melting-snowman May 10, 2022

deepesh-vasthimal-cko May 10, 2022

melting-snowman Apr 27, 2022

melting-snowman May 10, 2022

ehab-al-cko May 10, 2022

melting-snowman left a comment

melting-snowman Apr 28, 2022

ehab-al-cko May 10, 2022

deepesh-vasthimal-cko commented May 6, 2022

ehab-al-cko commented May 10, 2022

melting-snowman May 10, 2022

ehab-al-cko May 10, 2022

ehab-al-cko May 10, 2022

ehab-al-cko May 11, 2022

melting-snowman May 10, 2022

ehab-al-cko May 11, 2022

melting-snowman May 10, 2022

ehab-al-cko May 11, 2022

melting-snowman May 10, 2022

ehab-al-cko May 10, 2022

melting-snowman May 10, 2022

ehab-al-cko May 10, 2022

melting-snowman May 10, 2022

melting-snowman May 10, 2022

melting-snowman May 10, 2022

melting-snowman left a comment

ehab-al-cko commented May 10, 2022 •

edited

Loading

deepesh-vasthimal-cko left a comment

		@@ -1 +1 @@
		github "marmelroy/PhoneNumberKit" "3.3.3"
		github "marmelroy/PhoneNumberKit" "3.3.7"

PI-1738: Billing Form component: Standard #144

PI-1738: Billing Form component: Standard #144

Conversation

ehab-al-cko commented Apr 26, 2022 • edited Loading

Proposed changes

Types of changes

Checklist

Further comments

harry-brown-cko commented Apr 26, 2022

harry-brown-cko commented Apr 27, 2022 • edited Loading

harry-brown-cko left a comment • edited Loading

Choose a reason for hiding this comment

melting-snowman 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

melting-snowman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepesh-vasthimal-cko commented May 6, 2022

ehab-al-cko commented May 10, 2022

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

melting-snowman left a comment

Choose a reason for hiding this comment

ehab-al-cko commented May 10, 2022 • edited Loading

deepesh-vasthimal-cko left a comment

Choose a reason for hiding this comment

ehab-al-cko commented Apr 26, 2022 •

edited

Loading

harry-brown-cko commented Apr 27, 2022 •

edited

Loading

harry-brown-cko left a comment •

edited

Loading

ehab-al-cko commented May 10, 2022 •

edited

Loading