-
Notifications
You must be signed in to change notification settings - Fork 319
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
[Customer Center] Create CustomerCenterView
#3919
[Customer Center] Create CustomerCenterView
#3919
Conversation
CustomerCenterView
be92ded
to
ab9c5cf
Compare
RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift
Outdated
Show resolved
Hide resolved
RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift
Outdated
Show resolved
Hide resolved
RevenueCatUI/CustomerCenter/Views/ManageSubscriptionsView.swift
Outdated
Show resolved
Hide resolved
RevenueCatUI/CustomerCenter/Views/ManageSubscriptionsView.swift
Outdated
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.
Nice ship! Had some minor comments. I will let someone with more iOS knowhow approve though 😄
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.
I've only run the views via previews - is there a better way?
I wasn't able to see the Figma file, but have requested access.
To get it to compile I had to change LocalizedString
from let enUS: String
to let en_US: String
(I think the linter made this change, but we should ignore its rule in this case), and CustomerCenterData.FeedbackSurvey.Option
to CustomerCenterData.FeedbackSurveyOption
.
I think this is going to be a great and widely used new feature.
RevenueCatUI/CustomerCenter/Views/ManageSubscriptionsView.swift
Outdated
Show resolved
Hide resolved
RevenueCatUI/CustomerCenter/Views/ManageSubscriptionsView.swift
Outdated
Show resolved
Hide resolved
RevenueCatUI/CustomerCenter/Views/ManageSubscriptionsView.swift
Outdated
Show resolved
Hide resolved
I've been doing that as well, since making a purchase with PaywallsTester has been complicated. I was planning on looking into that today, so I can test deserialization, etc
Oops yes, that's on me. I fixed the linting issues and broke the compilation. Sorry about that cc @jamesrb1 |
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.
This is going to be so cool! 🤩
RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift
Outdated
Show resolved
Hide resolved
RevenueCatUI/CustomerCenter/Views/ManageSubscriptionsView.swift
Outdated
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.
looking great!!
RevenueCatUI/CustomerCenter/ViewModels/CustomerCenterViewModel.swift
Outdated
Show resolved
Hide resolved
RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift
Outdated
Show resolved
Hide resolved
RevenueCatUI/CustomerCenter/Views/ManageSubscriptionsView.swift
Outdated
Show resolved
Hide resolved
It looks like I still have to fix some availability checks, but this is good for another pass I think :) |
let dark: RCColor | ||
|
||
enum Mode: String { | ||
case system = "SYSTEM" |
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.
Should this also have light/dark modes?
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.
going to remove this for now, took this from the draft API and it is meant to be used later as an indication that it should be using the accent color from the app. Since we haven't defined the API completely and I am not using it I will remove it
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.
I am actually removing the whole appearance object, to keep things tidy
@ObservedObject | ||
private(set) var viewModel: ManageSubscriptionsViewModel | ||
|
||
let openURL: OpenURLAction |
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.
Hmm this seems unused? (Maybe you're planning to use it later though)
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.
Oh yes, I had a contact support button, that I removed because it's not possible to configure it, and this is leftovers. Removing!
@available(macOS, unavailable) | ||
@available(tvOS, unavailable) | ||
@available(watchOS, unavailable) | ||
class CustomerCenterViewModel: ObservableObject { |
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.
Strongly suggest making these @MainActor
😇
07422a6
to
973d7ec
Compare
RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift
Outdated
Show resolved
Hide resolved
case .success: | ||
self.refundRequestStatus = "Refund granted successfully!" | ||
case .userCancelled: | ||
self.refundRequestStatus = "Refund canceled" |
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.
We probably will need to localize these? I'm seeing that their usage is combined with another string so we would need to localize this (with String(localized: "")
and the other text where it's used (in order to avoid localizing all the combinations of these
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.
Separately, but I don't really like how these are localized with SwiftUI, since we don't have any compilation-time check that all the localizations are there... Maybe we should extract all these to strings enums so we have it better organized...
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.
right, I agree with the localization... I am surprised xcode doesn't warn or anything. We should come back to this because if I am not wrong it's the same mechanism in Paywalls right?
RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift
Show resolved
Hide resolved
I ended up removing |
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
027b480
to
83d4b1a
Compare
2858c16
to
8f21718
Compare
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.
We probably still need to handle localization and it would be great to have someone more familiar with SwiftUI take a look, but everything makes sense to me!
case missingPurchase = "MISSING_PURCHASE" | ||
case refundRequest = "REFUND_REQUEST" | ||
case changePlans = "CHANGE_PLANS" | ||
case cancel = "CANCEL" |
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.
Oh I was using lowercase for these in the backend... I'm ok changing them to uppercase though 👍
|
||
// swiftlint:disable:next todo | ||
// TODO: make configurable | ||
let urlString = "mailto:support@revenuecat.com?subject=\(encodedSubject)&body=\(encodedBody)" |
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.
We will be sending these from the backend soon. But we can address this todo in a separate PR 👍
Thank you @tonidero! Most of the hardcoded strings will go away since we are going to try to localize everything in the backend. So that's for future PRs. |
Going to merge it since the PRs are accumulating. @jamesrb1 feel free to make more comments that I can address in another PR if needed |
4ab05c3
into
integration/customer_support_workflow
Base branch is `integration/customer_support_workflow` so we don't merge into `main` yet Borrows a lot from #3865 Creates a new `CustomerCenterView` that can be used as a customer support workflow starting point. All details can be found in https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview --------- Co-authored-by: Will Taylor <wtaylor151@gmail.com> Co-authored-by: James Borthwick <109382862+jamesrb1@users.noreply.github.com>
Base branch is `integration/customer_support_workflow` so we don't merge into `main` yet Borrows a lot from #3865 Creates a new `CustomerCenterView` that can be used as a customer support workflow starting point. All details can be found in https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview --------- Co-authored-by: Will Taylor <wtaylor151@gmail.com> Co-authored-by: James Borthwick <109382862+jamesrb1@users.noreply.github.com>
Base branch is `integration/customer_support_workflow` so we don't merge into `main` yet Borrows a lot from #3865 Creates a new `CustomerCenterView` that can be used as a customer support workflow starting point. All details can be found in https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview --------- Co-authored-by: Will Taylor <wtaylor151@gmail.com> Co-authored-by: James Borthwick <109382862+jamesrb1@users.noreply.github.com>
Base branch is `integration/customer_support_workflow` so we don't merge into `main` yet Borrows a lot from #3865 Creates a new `CustomerCenterView` that can be used as a customer support workflow starting point. All details can be found in https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview --------- Co-authored-by: Will Taylor <wtaylor151@gmail.com> Co-authored-by: James Borthwick <109382862+jamesrb1@users.noreply.github.com>
Base branch is `integration/customer_support_workflow` so we don't merge into `main` yet Borrows a lot from #3865 Creates a new `CustomerCenterView` that can be used as a customer support workflow starting point. All details can be found in https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview --------- Co-authored-by: Will Taylor <wtaylor151@gmail.com> Co-authored-by: James Borthwick <109382862+jamesrb1@users.noreply.github.com>
Base branch is `integration/customer_support_workflow` so we don't merge into `main` yet Borrows a lot from #3865 Creates a new `CustomerCenterView` that can be used as a customer support workflow starting point. All details can be found in https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview --------- Co-authored-by: Will Taylor <wtaylor151@gmail.com> Co-authored-by: James Borthwick <109382862+jamesrb1@users.noreply.github.com>
Base branch is `integration/customer_support_workflow` so we don't merge into `main` yet Borrows a lot from #3865 Creates a new `CustomerCenterView` that can be used as a customer support workflow starting point. All details can be found in https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview --------- Co-authored-by: Will Taylor <wtaylor151@gmail.com> Co-authored-by: James Borthwick <109382862+jamesrb1@users.noreply.github.com>
Base branch is
integration/customer_support_workflow
so we don't merge intomain
yetBorrows a lot from #3865
Creates a new
CustomerCenterView
that can be used as a customer support workflow starting point.All details can be found in https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview