WIP: Extract card reader connection state management from App to Yosemite #12723
Conversation
…Yosemite framework.
…cies from App to Yosemite.
…ecessary parameters in the UI state.
jaclync
added
the
type: technical debt
Generated by 🚫 Danger |
|
|
| @@ -1,13 +1,68 @@ | |||
| import Combine | |||
| import Foundation | |||
| import UIKit | |||
| import Storage | |||
| import SwiftUI | |||
There was a problem hiding this comment.
We can also remove the SwiftUI import from here to assure that we have no UI-related stuff going on, at the moment it still compiles fine when we do 👍
| import Storage | ||
| import SwiftUI | ||
| import Yosemite | ||
|
|
||
| public enum CardReaderConnectionResult { |
There was a problem hiding this comment.
I know this is from before, but should we use a different name for this enum? *Result implies there's a result type to be expected, which is not the case, and later we find signatures like:
private var onCompletion: ((Result<CardReaderConnectionResult, Error>) -> Void)?
Perhaps something like CardReaderConnectionStatus?
| alertsPresenter: CardPresentPaymentAlertsPresenting, | ||
| alertsProvider: BluetoothReaderConnnectionAlertsProviding, |
| state = .idle | ||
| knownCardReaderProvider = knownReaderProvider | ||
| self.alertsPresenter = alertsPresenter | ||
| self.alertsProvider = alertsProvider | ||
| foundReaders = [] | ||
| knownReaderID = nil | ||
| skippedReaderIDs = [] |
There was a problem hiding this comment.
Since we're cleaning up the init here, maybe those should also go into some sort of "initial state" function?
|
|
||
| /// Facilitates connecting to a card reader | ||
| /// | ||
| final class CardReaderConnectionController { | ||
| public class CardReaderConnectionController { | ||
| public enum UIState { |
There was a problem hiding this comment.
This state machine here is looking great, I'd say ultimately since this is in Yosemite it shouldn't reference the UI in any manner, but some sort of CardReaderConnectionState instead. Then when we switch over the different cases in WooCommerce via connectionUIStateSubscription = controller.$cardReaderState is where we perform the specific calls to the alerts presenter which handles the UI bit
|
Thanks a lot for the feedback Gabriel, I agree with the suggestions and there is still a bunch of testing to be done. Though given the recent direction from leadership to not make significant changes that can affect the existing app, this PR seems unlikely to have a green light. I'm closing this PR for now but it'd be great to separate the UI logic if we have a chance in the future. |
Closes: #
Description
Testing instructions
Screenshots
RELEASE-NOTES.txtif necessary.