Skip to content

LOOP-1484: Correction ranges in therapy acceptance flow#12

Merged
novalegra merged 17 commits intodevfrom
novalegra/LOOP-1484
Jul 8, 2020
Merged

LOOP-1484: Correction ranges in therapy acceptance flow#12
novalegra merged 17 commits intodevfrom
novalegra/LOOP-1484

Conversation

@novalegra
Copy link
Contributor

@novalegra novalegra commented Jul 3, 2020

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:

  • Because the instantiated TidepoolService has access to the correct delegates for saving the settings to Loop, I moved the hook for the flow from the service setup view to the service settings view.
  • With design approval, I moved the titles for the screens in this flow to be in-body due to issues with buggy animations. This also leads to a more unified appearance, as the titles are in-body already for the settings editor views.

@novalegra novalegra requested review from mpangburn and ps2 July 3, 2020 17:39
@novalegra novalegra requested a review from rickpasetto July 7, 2020 19:24
Copy link
Contributor

@rickpasetto rickpasetto left a 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! 👍

weak var completionDelegate: CompletionDelegate?

let viewModel = PrescriptionCodeEntryViewModel()
var settingDelegate: ((TherapySettings) -> Void)?
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

@ps2 ps2 Jul 8, 2020

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.

Copy link
Contributor

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)

Copy link
Contributor

@ps2 ps2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

weak var completionDelegate: CompletionDelegate?

let viewModel = PrescriptionCodeEntryViewModel()
var settingDelegate: ((TherapySettings) -> Void)?
Copy link
Contributor

@ps2 ps2 Jul 8, 2020

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.

@novalegra novalegra merged commit 01f1a95 into dev Jul 8, 2020
@novalegra novalegra deleted the novalegra/LOOP-1484 branch July 8, 2020 20:20
ps2 added a commit that referenced this pull request Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants