nightscout / Trio

MIT License
64 stars 259 forks source link

Rework glucose dialog #139

Closed BrianWieder closed 3 months ago

BrianWieder commented 3 months ago

This ports https://github.com/Artificial-Pancreas/iAPS/pull/272 into Open-iAPS, which replaces the manual glucose entry pop-up with a sheet.

This is part of #47

Simulator Screenshot - iPhone 15 - 2024-04-30 at 22 45 28

Simulator Screenshot - iPhone 15 - 2024-04-30 at 22 45 31

bjornoleh commented 3 months ago

I tested the PR, and it works as intended.

However, it does not make sense to display mmol/L (or mg/dL) as header, and then repeat the units after each BG value. I am sure this is not what @dnzxy wanted (after reading the iAPS PR 272 discussions)

The minimal change that would make this look reasonable, is to replace the units in the header with "Glucose" (and make sure this is localised).

Screenshot from one of the suggestions in the iAPS PR:

image

Alternatively, only display units in the header, as was also presented in the other PR:

image

dnzxy commented 3 months ago

However, it does not make sense to display mmol/L (or mg/dL) as header, and then repeat the units after each BG value. I am sure this is not what @dnzxy wanted (after reading the iAPS PR 272 discussions)

The add glucose button belongs in the top right-hand corner of that view to adhere to iOS design guidelines. I had long discussions about this and could not get it in. Personally, I'm all for omitting the unit. It's clear from the app settings which unit was applied, so it is not necessary to display it in every row.

Personally, I also like the inverse (first value, then time), but that's a personal preference. It's how I have it in my personal build.

bjornoleh commented 3 months ago

Thanks. I also notice that the unique ID is displayed for every reading, this is not meant for humans to read, and can safely be omitted too.

BrianWieder commented 3 months ago

Ah now knowing its alright if we deviate a bit from the original PRs, I will make some further changes. I am going to close this PR and combine this and the port of https://github.com/Artificial-Pancreas/iAPS/pull/314 into one PR.