Skip to content
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-1486: add suspend threshold information screen + editor #14

Merged
merged 48 commits into from
Jul 13, 2020

Conversation

novalegra
Copy link
Contributor

@novalegra novalegra commented Jul 10, 2020

https://tidepool.atlassian.net/browse/LOOP-1486

This PR also makes TherapySettings be a subcomponent of a MockPrescription

@novalegra novalegra requested review from ps2 and rickpasetto and removed request for ps2 July 10, 2020 14:38
@novalegra novalegra closed this Jul 10, 2020
@novalegra novalegra deleted the novalegra/LOOP-1486 branch July 10, 2020 14:39
@novalegra novalegra restored the novalegra/LOOP-1486 branch July 10, 2020 14:39
@novalegra novalegra reopened this Jul 10, 2020
@@ -45,65 +45,20 @@ public struct MockPrescription: Codable {
public let cgm: CGMType // CGM type (manufacturer & model)
public let pump: PumpType // Pump type (manufacturer & model)
public let bloodGlucoseUnit: BGUnit
public let basalRateSchedule: BasalRateSchedule
Copy link
Contributor Author

@novalegra novalegra Jul 10, 2020

Choose a reason for hiding this comment

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

These changes are to make TherapySettings be a subcomponent of a prescription

@novalegra novalegra requested review from nhamming and removed request for ps2 July 10, 2020 18:36
Copy link
Contributor

@nhamming nhamming left a comment

Choose a reason for hiding this comment

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

No concerns, but a couple of questions to better understand what is happening.

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

Choose a reason for hiding this comment

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

Curious if this blocks the scrolls of the title up into the navigation bar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .never would do that if there was an actual title in the settings editor screens, but the settings editors' titles are actually empty strings, so they appear the same

Comment on lines +143 to +147
guard let prescription = viewModel.prescription else {
// Go back to code entry step if we don't have prescription
let view = PrescriptionCodeEntryView(viewModel: viewModel)
return DismissibleHostingController(rootView: view)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is curious to me. Is this a possible entry point of the workflow or can the user just jump to this point? Just trying to understand how the user can get here without the needed information, which then bumps them to the start of the workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user shouldn't be able to get here without a prescription; this is primarily to unwrap it (since it's an optional in the view model) and to catch anything weird that could have happened with the Tidepool backend.

@novalegra novalegra merged commit 1f2bd43 into dev Jul 13, 2020
@novalegra novalegra deleted the novalegra/LOOP-1486 branch July 13, 2020 20:33
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.

2 participants