-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose Customer Center to UIKit #4560
Conversation
RevenueCatUI/CustomerCenter/Views/UIKit Compatibility/CustomerCenterViewController.swift
Show resolved
Hide resolved
@RCGitBot please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Left a few small comments.
There's a decent amount of extra layout stuff we ended up doing for Paywalls View Controller for proper layout in hybrids, that I imagine we'll likely want to apply for Customer Center too sooner or later.
But we can think about extracting out the logic into a subclass and then using it for both later, I don't believe it'd lead to breaking changes
@@ -39,6 +39,7 @@ public struct CustomerCenterView: View { | |||
/// - Parameters: | |||
/// - customerCenterActionHandler: An optional `CustomerCenterActionHandler` to handle actions | |||
/// from the customer center. | |||
/// - mode: The presentation mode for the customer center. Defaults to `.default` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
|
||
#if canImport(UIKit) && os(iOS) | ||
|
||
/// Warning: This is currently in beta and subject to change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 let's not even have this. We really shouldn't be changing the UI, and we're going GA very soon in any case.
|
||
/// Warning: This is currently in beta and subject to change. | ||
/// | ||
/// A SwiftUI view for displaying a customer support common tasks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// A SwiftUI view for displaying a customer support common tasks | |
/// A UIKit ViewController for displaying a customer support common tasks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also that docstring's copy could generally be improved, "displaying a customer support common tasks" sounds broken.
I realize that this is likely copy-pasted from SwiftUI, maybe we do a follow-up PR improving both sides
/// Create a view controller to handle common customer support tasks | ||
/// - Parameters: | ||
/// - customerCenterActionHandler: An optional `CustomerCenterActionHandler` to handle actions | ||
/// from the customer center. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we should brand this and go always Customer Center
rather than customer center
RevenueCatUI/CustomerCenter/Views/UIKit Compatibility/CustomerCenterViewController.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a decent amount of extra layout stuff we ended up doing for Paywalls View Controller for proper layout in hybrids, that I imagine we'll likely want to apply for Customer Center too sooner or later.
Most of that was to support footer paywalls so hopefully it should be much simpler in this case. We should also decide whether we want to support both a "presentCustomerCenter" style API and a declarative "CustomerCenterView" in the hybrids or only one of them... Mostly this has to do with the "dismissal" mechanism. But for now, this PR looks ok to me!
@RCGitBot please test |
Description
This PR:
CustomerCenterViewController
classCustomerCenterViewController
in the PaywallTester appTesting
CustomerCenterViewController
in the PaywallTester app