-
Notifications
You must be signed in to change notification settings - Fork 7
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
LOOP-1484: Correction ranges in therapy acceptance flow #12
Conversation
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.
Some minor comments, but otherwise LGTM! 👍
@@ -27,14 +34,14 @@ enum PrescriptionReviewScreen { | |||
class PrescriptionReviewUICoordinator: UINavigationController, CompletionNotifying, UINavigationControllerDelegate { | |||
var screenStack = [PrescriptionReviewScreen]() | |||
weak var completionDelegate: CompletionDelegate? | |||
|
|||
let viewModel = PrescriptionCodeEntryViewModel() | |||
var settingDelegate: ((TherapySettings) -> Void)? |
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 this is just a function, it is not really a "Delegate". Consider calling this onSettingFinished
.
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 a way to save the settings to Loop. Perhaps I'm not understanding the definition of a delegate, but wouldn't it be a delegate in this case because it's interacting with another object to coordinate state?
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.
Whoever is implementing this is a delegate for saving, but I agree with Rick that since it references a function, and not the delegate itself. Not sure if "save" or "finish" is the right verb. saveTherapySettings
or 'onReviewFinished` both seem reasonable, depending on what you intend as possible uses for this function.
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, what @ps2 said :). (Any of his suggested functions are fine with me)
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.
LGTM!
@@ -27,14 +34,14 @@ enum PrescriptionReviewScreen { | |||
class PrescriptionReviewUICoordinator: UINavigationController, CompletionNotifying, UINavigationControllerDelegate { | |||
var screenStack = [PrescriptionReviewScreen]() | |||
weak var completionDelegate: CompletionDelegate? | |||
|
|||
let viewModel = PrescriptionCodeEntryViewModel() | |||
var settingDelegate: ((TherapySettings) -> Void)? |
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.
Whoever is implementing this is a delegate for saving, but I agree with Rick that since it references a function, and not the delegate itself. Not sure if "save" or "finish" is the right verb. saveTherapySettings
or 'onReviewFinished` both seem reasonable, depending on what you intend as possible uses for this function.
This PR adds an instructional screen & an editor screen for the correction range setting, in addition to the framework (delegates, etc) needed to save the prescription to Loop.
A few notes: