Skip to content

LOOP-1628: fix ordering & update guardrail correctly#18

Merged
novalegra merged 3 commits into
devfrom
novalegra/LOOP-1628
Jul 20, 2020
Merged

LOOP-1628: fix ordering & update guardrail correctly#18
novalegra merged 3 commits into
devfrom
novalegra/LOOP-1628

Conversation

@novalegra

Copy link
Copy Markdown
Contributor

@novalegra novalegra requested review from mpangburn and rickpasetto and removed request for rickpasetto July 17, 2020 18:29

init(model: TherapySettingsViewModel){
precondition(model.therapySettings.glucoseUnit != nil)
precondition(model.therapySettings.glucoseTargetRangeSchedule?.scheduleRange() != nil)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[nit] can simplify the precondition to being on the optionality of glucoseTargetRangeSchedule itself, since scheduleRange()'s return value is non-optional

Comment on lines +30 to +37
maxValue: [
viewModel.therapySettings.glucoseTargetRangeSchedule?.minLowerBound().doubleValue(for: unit),
viewModel.therapySettings.preMealTargetRange?.minValue,
viewModel.therapySettings.workoutTargetRange?.minValue
]
.compactMap { $0 }
.min()
.map { HKQuantity(unit: unit, doubleValue: $0) },

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wonder if it's worth extracting this logic to LoopKit so it can be shared with Loop (since the concept of suspend threshold now exists in LoopKit anyway)

I'm not sure where it would live.. maybe just a static function on Guardrail?

extension Guardrail where Value == HKQuantity {
    static func maxSuspendThresholdValue(correctionRangeSchedule: GlucoseRangeSchedule, preMealTargetRange: DoubleRange, workoutTargetRange: DoubleRange)
}

WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That makes sense so we're not duplicating it in multiple places

@rickpasetto rickpasetto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some comments for consideration, but other than that LGTM

}
let view = CorrectionRangeInformationView(onExit: onExit)
let hostedView = DismissibleHostingController(rootView: view)
hostedView.navigationItem.largeTitleDisplayMode = .always // TODO: hack to fix jumping, will be removed once editors have titles

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting find! I will try using this on my own screens, as I'm seeing that jumping, too.

}
settingsViewModel.didFinishStep = didFinishStep

screenStack = [.enterCode]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this always supposed to be first now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, from the discussions with design/product it seems that the therapy settings flow will be restarted from the beginning if the app quits

@novalegra novalegra merged commit 16a0b46 into dev Jul 20, 2020
@novalegra novalegra deleted the novalegra/LOOP-1628 branch July 20, 2020 17:25
ps2 added a commit that referenced this pull request Sep 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