PI-1738: Billing Form component: Standard

[ehab-al-cko]

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]

Base branch should be 4.0.0_RC

[harry-brown-cko]

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)

[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.

[melting-snowman]

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

[melting-snowman]

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.

[deepesh-vasthimal-cko]

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

[melting-snowman]

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 (///)

[melting-snowman]

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.

[ehab-al-cko]

Fixed

[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

[melting-snowman]

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?

[ehab-al-cko]

Resolved on slack

[deepesh-vasthimal-cko]

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]

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

Fixed

[melting-snowman]

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.

[ehab-al-cko]

-> Revert to 3.3.3

[ehab-al-cko]

Should be reverted to 3.3.3

[ehab-al-cko]

Done

[melting-snowman]

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

[ehab-al-cko]

Done

[melting-snowman]

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?

[ehab-al-cko]

TODO: a new ticket to refactor and test this change

[melting-snowman]

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

[ehab-al-cko]

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

[melting-snowman]

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.

[ehab-al-cko]

Feedback from @patrick-hoban-cko

[melting-snowman]

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]

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]

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.

[ehab-al-cko]

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