-
Notifications
You must be signed in to change notification settings - Fork 334
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 a completion handler #4028
[Customer Center] Create a completion handler #4028
Conversation
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. |
Generated by 🚫 Danger |
9efea65
to
4569a03
Compare
f6c8421
to
0bd1880
Compare
4569a03
to
647fd79
Compare
0bd1880
to
e2bf8b8
Compare
647fd79
to
76e80a6
Compare
e2bf8b8
to
07c65ee
Compare
76e80a6
to
68385b1
Compare
07c65ee
to
73696c6
Compare
68385b1
to
2af82b5
Compare
73696c6
to
21ad1e0
Compare
2af82b5
to
19fa733
Compare
21ad1e0
to
7a91e51
Compare
19fa733
to
37cd710
Compare
7a91e51
to
d218ffc
Compare
37cd710
to
a1b2611
Compare
d218ffc
to
6ff8e66
Compare
a1b2611
to
7f1d724
Compare
6ff8e66
to
9a3c5c6
Compare
7f1d724
to
9976082
Compare
9a3c5c6
to
02eed7c
Compare
9976082
to
2afffc0
Compare
02eed7c
to
5afee3b
Compare
2afffc0
to
2986865
Compare
5afee3b
to
f78fe44
Compare
2986865
to
7ccc4f5
Compare
f78fe44
to
f6621c6
Compare
7ccc4f5
to
20e1603
Compare
f6621c6
to
458fa4c
Compare
20e1603
to
4aeabf3
Compare
458fa4c
to
1dfd58a
Compare
4aeabf3
to
f0a9752
Compare
1dfd58a
to
d1173d1
Compare
f0a9752
to
e9a8262
Compare
d1173d1
to
d6eca04
Compare
import Foundation | ||
import SwiftUI | ||
|
||
final class CustomerCenterCompletionHandler: 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.
I've been wondering whether it would make more sense to call this CustomerCenterActionHandler
. Then, we would notify whenever an action is performed.
My main concern with this approach is that it moves away from what we have in paywalls, where we have individual callbacks for many actions like purchase/restore started/failed/completed... But I feel since there are going to be more actions here, it makes more sense to do something like that. Wdyt?
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.
That's a good point... Right, I think at some point we will have actions that are no completion actions like feedbackSurveyStarted
so we want this to be an action handler. I honestly prefer just having this handler with a bunch of different events that can be handled in a switch statement than a lot of functions like we do in paywalls
It is true that adding a new event to a sealed class is technically a breaking change, but I think it's assumable. It is also true that it's not possible to modify an existing case in an enum (and add more properties) without creating a new event, but that's also an issue with independent functions.
In summary I find it cleaner to use sealed class when we are going to have a lot of potential events.
|
||
} | ||
|
||
struct CustomerCenterResult: Equatable { |
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 we follow my previous suggestion, I would move away from using the Result
naming, since this may happen at any point, and even multiple times. I would only keep this naming if we want to return a single result after the customer center closes, but I think I prefer my suggested option. Lmk what you think!
@StateObject | ||
private var viewModel = CustomerCenterViewModel() | ||
@StateObject | ||
private var completionHandler: CustomerCenterCompletionHandler = .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.
Would it be possible to move this inside the view model? That way we centralize the logic there.
@@ -200,6 +209,7 @@ struct ManageSubscriptionsButtonsView: View { | |||
Task { | |||
openURL(URLUtilities.createMailURL()!) | |||
} | |||
completionHandler.supportContacted() |
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.
Ideally this calls the view model, which internally calls whatever it needs I think.
e9a8262
to
43ded08
Compare
d6eca04
to
f51f583
Compare
43ded08
to
a9430db
Compare
f51f583
to
c50e65e
Compare
a9430db
to
f4116bb
Compare
Checklist
purchases-android
and hybridsMotivation
Description