MBL-1014: Replace ReactiveSwift in ReportProjectFormViewModel with Combine #1873
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,33 +7,32 @@ | |
| } | ||
|
|
||
| struct ReportProjectFormView: View { | ||
| @Binding var popToRoot: Bool | ||
| let projectID: String | ||
| let projectURL: String | ||
| let projectFlaggingKind: GraphAPI.FlaggingKind | ||
|
|
||
| @SwiftUI.Environment(\.dismiss) private var dismiss | ||
| @StateObject private var viewModel = ReportProjectFormViewModel() | ||
|
There was a problem hiding this comment. @scottkicks OK, just learned something interesting. In a truly react-like way, There was a problem hiding this comment. This also means we can't pass in state directly on init of the viewmodel, or pass the viewmodel directly into the view...so I set some properties on the viewmodel in |
||
|
|
||
| @State private var showLoading: Bool = false | ||
|
There was a problem hiding this comment. Now that the view model is using Combine, we can bind the state of the view model directly to the SwiftUI view. That means we don't need these extra state variables in the view, or any of the There was a problem hiding this comment. I see your point. This pattern makes me wonder what the benefit of @State is. In my experience, SwiftUI views typically handle UI specific state, for the most part, and then only pass what the VM needs when it's needed. Once save is pressed for example. I like the idea of having the VM manage non UI specific state like saveEnabled, bannerMessage, possibly showLoading, etc., but does it make sense for it to own the more UI specific state like retrievedEmail and details? There was a problem hiding this comment. Yeah, I think that's a really interesting question, and I'm not sure I have a great answer for it, either. The way we've structured things here, the viewmodel actually has no knowledge of the view directly, so at there would be no way for it to even ask the view for its I think There was a problem hiding this comment. At present, my understanding of There was a problem hiding this comment. I agree with this - the default should be for the viewmodel to own things (unless it's literally something that only affects the UI). Any time logic can live in the viewmodel instead of the view, it should. |
||
| @FocusState private var focusField: ReportFormFocusField? | ||
|
|
||
| var body: some View { | ||
| GeometryReader { proxy in | ||
| Form { | ||
| SwiftUI.Section(Strings.Email()) { | ||
| if let retrievedEmail = viewModel.retrievedEmail, !retrievedEmail.isEmpty { | ||
| Text(retrievedEmail) | ||
| .font(Font(UIFont.ksr_body())) | ||
| .foregroundColor(Color(.ksr_support_400)) | ||
| .disabled(true) | ||
| } else { | ||
| Text(Strings.Loading()) | ||
|
There was a problem hiding this comment. Added this because it makes the UI feel a little smoother. When the e-mail loads, the layout of the UI won't flicker. |
||
| .font(Font(UIFont.ksr_body())) | ||
| .foregroundColor(Color(.ksr_support_400)) | ||
| .italic() | ||
| .disabled(true) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -45,7 +44,7 @@ | |
| } | ||
|
|
||
| SwiftUI.Section { | ||
| TextEditor(text: $viewModel.detailsText) | ||
|
There was a problem hiding this comment. Here's an example of a two-way binding - There was a problem hiding this comment. My only thought here is that the view could manage state like this until it is ready to be passed to the view model in its final form There was a problem hiding this comment. I could see us passing state back to the viewmodel with an input method like Based on what I understand right now - a but if Playing with how this is organized seems like a really good thing to mess around with during a pairing session, it'd be interesting to see what you mean and figure out how else it might work. |
||
| .frame(minHeight: 75) | ||
| .font(Font(UIFont.ksr_body())) | ||
| .focused($focusField, equals: .details) | ||
|
|
@@ -59,52 +58,35 @@ | |
| .toolbar { | ||
| ToolbarItem(placement: .navigationBarTrailing) { | ||
| LoadingBarButtonItem( | ||
| saveEnabled: $viewModel.saveButtonEnabled, | ||
| saveTriggered: $viewModel.saveTriggered, | ||
|
There was a problem hiding this comment. So this is an interesting one - I'm not actually sure that it makes sense for a However, it's not really a state, it's more something that happens. It would make more sense to me to model it internally as a stream of voids, where only one event occurs when the button is pressed. I actually like Apple's pattern here with the underlying View class There was a problem hiding this comment. Good callout. This makes more sense to me too! There was a problem hiding this comment. I like the closure approach better, too! There was a problem hiding this comment. Cool, I'll throw myself a quick ticket to refactor this. Just to keep this PR a bit more svelte. |
||
| showLoading: $showLoading, | ||
| titleText: Strings.Send() | ||
| ) | ||
| } | ||
| } | ||
| .onAppear { | ||
| focusField = .details | ||
|
|
||
| viewModel.projectID = projectID | ||
| viewModel.projectFlaggingKind = projectFlaggingKind | ||
|
|
||
| viewModel.inputs.viewDidLoad() | ||
| } | ||
| .onReceive(viewModel.$bannerMessage) { newValue in | ||
| showLoading = false | ||
|
There was a problem hiding this comment. It might be cleaner to move the banner logic into the ViewModel, so that the submit, banner display, timeout, and dismiss all happen in the same place. However, that was going to be a bigger piece of refactoring, so I left this as-is. There was a problem hiding this comment. Yea, that could be cleaner. What if we moved showLoading along with it? There was a problem hiding this comment. Yeah I think that'd be good too |
||
|
|
||
| /// bannerMessage is set to nil when its done presenting. When it is done, and submit was successful, dismiss this view. | ||
| if newValue == nil, viewModel.submitSuccess { | ||
| dismiss() | ||
| popToRoot = true | ||
| } | ||
| } | ||
| .onReceive(viewModel.$saveTriggered) { triggered in | ||
| showLoading = triggered | ||
| } | ||
| .overlay(alignment: .bottom) { | ||
| MessageBannerView(viewModel: $viewModel.bannerMessage) | ||
| .frame( | ||
| minWidth: proxy.size.width, | ||
| idealWidth: proxy.size.width, | ||
|
|
||
Codecov | Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import Combine | ||
| import Foundation | ||
|
|
||
| public final class CombineTestObserver<Value, Error: Swift.Error> { | ||
|
There was a problem hiding this comment. There's another ticket for fleshing this out. This is just the barest-bones stub to get the tests working. |
||
| public private(set) var events: [Value] = [] | ||
| private var subscriptions = Set<AnyCancellable>() | ||
|
|
||
| public func observe(_ publisher: any Publisher<Value, Error>) { | ||
| publisher.sink { _ in | ||
| // TODO(MBL-1017) implement this as part of writing a new test observer for Combine | ||
| fatalError("Errors haven't been handled here yet.") | ||
| } receiveValue: { [weak self] value in | ||
| self?.events.append(value) | ||
| } | ||
| .store(in: &self.subscriptions) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import Combine | ||
| import Foundation | ||
| import ReactiveSwift | ||
|
|
||
| extension Signal where Error == Never { | ||
|
There was a problem hiding this comment. This was an experiment in bridging code that I'm not using any more. Kind of nifty though. |
||
| var combinePublisher: AnyPublisher<Value, Never> { | ||
| let subject = PassthroughSubject<Value, Never>() | ||
| self.observeValues { value in | ||
| subject.send(value) | ||
| } | ||
|
|
||
| return subject.eraseToAnyPublisher() | ||
| } | ||
|
|
||
| public func assign(toCombine published: inout Published<Value>.Publisher) { | ||
| self.combinePublisher.assign(to: &published) | ||
| } | ||
| } | ||
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 moved these properties into the view model for two reasons:
projectIdandprojectFlaggingKindto submit the report project mutationThere 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.
Agreed. I mentioned in a comment below that we could consider injecting the VM with these properties from the outset instead of having the view be something of a middleman here.