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

Utilize max bolus limit in Bolus module #100

Merged
merged 4 commits into from
May 3, 2024

Conversation

MikePlante1
Copy link
Contributor

@MikePlante1 MikePlante1 commented Apr 14, 2024

In the bolus module, if bolus amount is set over the Max Bolus set in Pump Settings, disable the Enact bolus button, and change its text to Max Bolus exceeded! (>X) where X is the value of the Max Bolus setting.

Max Bolus is set to 10U for this test:

Screenshot 2024-04-30 at 8 25 41 PM

Cherry-picked from Artificial-Pancreas/iAPS@581e3e2 and Artificial-Pancreas/iAPS@7c7cafe but did not include external insulin parts since that no longer exists here.

This PR replaces #73 which was accidentally closed.

MikePlante1 and others added 2 commits April 9, 2024 17:15
In the bolus module, if bolus amount is set over the `Max Bolus` set in `Pump Settings', disable the `Enact bolus` button, change it's text to `Max Bolus exceeded!`, and change alert dialog for external insulin.

If bolus amount is set over 3x Max Bolus, disable the external insulin button as well.

Cherry-picked from Artificial-Pancreas/iAPS@581e3e2 and Artificial-Pancreas/iAPS@7c7cafe but had to refactor the alert to get it to build

Co-Authored-By: Jon B Mårtensson <53905247+jon-b-m@users.noreply.github.com>
@bjornoleh
Copy link
Contributor

Could we display the actual max bolus limit here? E.g something like “Max bolus (10U) exceeded”

Also, perhaps use lowercase for “bolus”?

I have not looked at the code or built at this time.

@MikePlante1
Copy link
Contributor Author

Could we display the actual max bolus limit here? E.g something like “Max bolus (10U) exceeded”

Also, perhaps use lowercase for “bolus”?

I have not looked at the code or built at this time.

We could, but there are already a bunch of localized strings for Max Bolus exceeded!. I suppose Max Bolus exceeded! (>10U) would be doable, but I'm not sure how much value that adds.

The reason Max Bolus is capitalized and exceeded is not is because Max Bolus is the name of the setting.

@bjornoleh
Copy link
Contributor

I think the reason I’d like to see the actual max bolus setting here is that (if I remember correctly, I haven’t built this yet), the blousing is blocked until you enter an amount below that max setting. If you don’t remember what that was, it could take some trial and error to get to the bolusing (if you act wanted to bolus near the limit).

Before these changes, the app would just (secretly) round down to the
max bolus setting, and issue it straight away. Not ideal, of course, but the implementation in the PR adds some faff when entering something just above the max limit.

@dsnallfot
Copy link
Contributor

dsnallfot commented Apr 15, 2024

Just want to share a visualization on how I’ve implemented this in my custom branch. Think it adds some UX value to include the actual numbers. Also the same logic for maxcarbs exceeded In meal view

maxbolus maxcarbs
IMG_3605 IMG_3604

@dnzxy
Copy link
Contributor

dnzxy commented Apr 15, 2024

Guys, sorry to be the one to point this out: this PR is not about UI redesigning or adding new fancier UI elements or warnings, it’s about bringing a limit into the app that is missing, very much needed for safety, and was added between iAPS 2.3.x and 3.x in dev.

Can we please see this through and add all (very good!) ideas around a better UI to the project board?

@bjornoleh
Copy link
Contributor

@dnzxy Agreed regarding bigger changes like the screenshots above. But my initial suggestion was just to add printing of a variable (max bolus) in the one string. I think that’s not really in the fancy UI department 😊

@dnzxy
Copy link
Contributor

dnzxy commented Apr 15, 2024

If I‘m not mistaken (please correct me @MikePlante1 or Bjørn), this means having to change all the i18n strings to cater for the string literals of the max bolus variable value. Is that worth it for now if we are probably going to redesign most of this stuff anyways and will very likely be needing new translations in the process, too? If it’s easy and quick to do, let’s please add/change it 🤝.

@bjornoleh
Copy link
Contributor

It’s very quick to copy paste in the English/english for all strings, meaning the translation for the string will be lost, but that’s a minor thing once we have a translation project up (probably Crowdin). We can also quickly get the languages that we collectively master correctly translated here before release too.

@MikePlante1
Copy link
Contributor Author

@bjornoleh I don't really think it's needed, but I could add (>10U) to the end of 'Max Bolus exceeded!` if you think it'd be beneficial. I'd just rather leave that localized string intact, at least for now.

@MikePlante1
Copy link
Contributor Author

^Obviously the 10 would change to whatever is set in ⚙️>Pump Settings>Max Bolus.

@bjornoleh
Copy link
Contributor

I would still like to see the max bolus setting in the string that currently says "Max Bolus exceeded!". But I don't see exactly how to do this myself just yet, and I won't find time to look closer during the next few days.

Great if someone else can do this, but if not, I'm happy to accept the PR as is for now.

@MikePlante1
Copy link
Contributor Author

I would still like to see the max bolus setting in the string that currently says "Max Bolus exceeded!". But I don't see exactly how to do this myself just yet, and I won't find time to look closer during the next few days.

Great if someone else can do this, but if not, I'm happy to accept the PR as is for now.

Is Max Bolus Exceeded! (>10U) an acceptable formatting to you? 10 being whatever your set maxBolus is that you’ve exceeded

@bjornoleh
Copy link
Contributor

I would still like to see the max bolus setting in the string that currently says "Max Bolus exceeded!". But I don't see exactly how to do this myself just yet, and I won't find time to look closer during the next few days.
Great if someone else can do this, but if not, I'm happy to accept the PR as is for now.

Is Max Bolus Exceeded! (>10U) an acceptable formatting to you? 10 being whatever your set maxBolus is that you’ve exceeded

Yes, I think this would be great!

also revert duplicate localization (thanks @bjornoleh )
@MikePlante1
Copy link
Contributor Author

@bjornoleh The max bolus string has now been updated. I confirmed localizations still work with it by selecting Deutsch in a simulator. I also deleted the duplicate localization you pointed out.

@bjornoleh
Copy link
Contributor

Thanks @MikePlante1! Merging this with two approvals.

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.

4 participants