Frames SDK API specs comments #166
Conversation
| @@ -18,7 +18,7 @@ public struct BillingFormFactory { | |||
| .phoneNumber(DefaultBillingFormPhoneNumberCellStyle())] | |||
| } | |||
|
|
|||
| static func getBillingFormViewController(style: BillingFormStyle?, data: BillingForm?, delegate: BillingFormViewModelDelegate) -> (BillingFormViewModelDelegate?, UINavigationController)? { | |||
| static func getBillingFormViewController(style: BillingFormStyle?, data: BillingForm?, delegate: BillingFormViewModelDelegate) -> (deleagte: BillingFormViewModelDelegate?, navigationController: UINavigationController)? { | |||
There was a problem hiding this comment.
| static func getBillingFormViewController(style: BillingFormStyle?, data: BillingForm?, delegate: BillingFormViewModelDelegate) -> (deleagte: BillingFormViewModelDelegate?, navigationController: UINavigationController)? { | |
| static func getBillingFormViewController(style: BillingFormStyle?, data: BillingForm?, delegate: BillingFormViewModelDelegate) -> (delegate: BillingFormViewModelDelegate?, navigationController: UINavigationController)? { |
There was a problem hiding this comment.
I'm late for this, but why are we passing in a delegate and then also giving it back out. Doesn't the caller already own it since it provided it in the first place?
There was a problem hiding this comment.
Agree with @alex-ioja-yang-cko , looking at the code i see getBillingFormViewController takes in a delegate which is of the view controller where the billingVC is presented. It is then passed to DefaultBillingFormViewModel where it has the delegate methods and sets this delegate.
But from the sample app when we call getBillingFormViewController as merchant it is never used inside the VC itself ( when returned ) and we can get rid of the returned delegate if we are not using it.
There was a problem hiding this comment.
we should address this and/or fix the typo
There was a problem hiding this comment.
It was a comment on old PR to expose delegate
There was a problem hiding this comment.
I think you misinterpreted what that comment asked for. It wasn't for a delegate to be returned alongside the VC, it wouldn't make sense to if you have no need for it.
I was saying that the common syntax is to assign the delegate after the object has been created, not during the initialisation. The common way to see delegate being assigned is in patterns of:
let object = MyObject()
object.delegate = myDelegaterather than
let object = MyObject(delegate: myDelegate)There are some nuances to why that is, which are not applicable here, but make it a more regular occurance when navigating the code. I would have rather you rejected this idea than add this code change :).
| @@ -18,7 +18,7 @@ public struct BillingFormFactory { | |||
| .phoneNumber(DefaultBillingFormPhoneNumberCellStyle())] | |||
| } | |||
|
|
|||
| static func getBillingFormViewController(style: BillingFormStyle?, data: BillingForm?, delegate: BillingFormViewModelDelegate) -> (BillingFormViewModelDelegate?, UINavigationController)? { | |||
| static func getBillingFormViewController(style: BillingFormStyle?, data: BillingForm?, delegate: BillingFormViewModelDelegate) -> (deleagte: BillingFormViewModelDelegate?, navigationController: UINavigationController)? { | |||
There was a problem hiding this comment.
we should address this and/or fix the typo
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.