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

Enable editing of preset temp targets #236

Merged
merged 8 commits into from
May 30, 2024
Merged

Conversation

dsnallfot
Copy link
Contributor

@dsnallfot dsnallfot commented May 25, 2024

  • Enable editing Name, Target and Duration for preset temp targets
  • Swipe left to edit or delete
  • Replaced display of "Low target - High target" in presetView since only low target is used
  • Edit popup shows current settings, and editing of name, target and duration is done in the popup

Connected to issue #234 and PR #235

Extra useful when preset temp targets are used with shortcuts (no need to re add changed presets in shortcuts since id remains unchanged after edit - even if you change name on the preset)

NOTE! This is not an attempt to redesign the entire UI. This is just to add edit functionality with minimal changes.

Screenshots

Edit or Delete swipe Edit View
Simulator Screenshot - iPhone 13 mini+ - 2024-05-25 at 09 46 13 Simulator Screenshot - iPhone 13 mini+ - 2024-05-25 at 09 46 19

Video demo

Simulator.Screen.Recording.-.iPhone.13.mini+.-.2024-05-25.at.09.45.30.mp4

- Enable editing Name, Target and Duration for preset temp targets
- Swipe left to edit or delete
- Replaced display of "Low target - High target" in presetView since only low target is used
@dsnallfot
Copy link
Contributor Author

dsnallfot commented May 25, 2024

Bug: just figured out that md/dl values are saved as decimals instead of integers when editing presets, which causes unwanted decimals in temptargets_presets.json file when mmol is converted to mg/dl and not rounded. Will fix this shortly.

@dnzxy
Copy link
Contributor

dnzxy commented May 25, 2024

Please see my comment on #235, the same is applies for the UI of this proposed change.

@dsnallfot dsnallfot closed this May 25, 2024
@dsnallfot
Copy link
Contributor Author

dsnallfot commented May 25, 2024

Closed since we should wait with all UI/UX changes until after 1.0 release.

@dsnallfot dsnallfot changed the title Enable editing of preset temp targets Enable editing of preset temp targets (Draft to be revisited after 1.0 release) May 25, 2024
@dsnallfot dsnallfot reopened this May 25, 2024
@aug0211
Copy link
Contributor

aug0211 commented May 25, 2024

I like this UX, it’s more intuitive than the override version (editing in the popup is more natural to other experiences on iOS)

I think swiping is clear enough, better than a long tap! I guess a dedicated “Edit” mode could be added in the top corner; maybe that’s more iOS standard? I like the swipe for options though, personally. I could see additional swipe functionality being added eventually, too, such as “duplicate” to save into a new entry with identical settings as the starting point to modify.

I’m very eager to get people who are ready and able to do UX and UI work unblocked - UI/UX is perhaps the weakest point of Trio currently (maybe behind Caregiver experience) 🙌

@dsnallfot
Copy link
Contributor Author

Reopen and put in the "idea box" until after 1.0 release

@dsnallfot
Copy link
Contributor Author

The mg/dl decimal rounding bug should be fixed now

@dsnallfot
Copy link
Contributor Author

The same alert after swiping delete logic as in profile overrides PR now also implemented for TTs in this PR

Simulator.Screen.Recording.-.iPhone.13.mini+.-.2024-05-26.at.10.08.55.mp4

@dsnallfot dsnallfot marked this pull request as ready for review May 26, 2024 10:15
@dnzxy dnzxy changed the title Enable editing of preset temp targets (Draft to be revisited after 1.0 release) Enable editing of preset temp targets May 26, 2024
@dnzxy dnzxy added the enhancement New feature or request label May 26, 2024
@avouspierre
Copy link
Contributor

The edit mode doesn't allow to use the experimental approach. Probably not useful as asked in #154

@dsnallfot
Copy link
Contributor Author

dsnallfot commented May 26, 2024

The edit mode doesn't allow to use the experimental approach. Probably not useful as asked in #154

Thanks Pierre, Good spotted! You can edit temp targets set by the slider, but only target and duration (no experimental hbt slider in edit mode. That was a compromise I choose to make but didn't communicate clearly in the notes)

But I also noticed when testing right now this that I need to fix the mgdl-mmol conversion for targets set by slider to be able to edit them correctly. Right now only mgdl works as intended. Will fix tonite regardless of what happens with PR 154

@dnzxy
Copy link
Contributor

dnzxy commented May 26, 2024

There’s quite a lot of discussion in #154 and I don’t think the slider should be removed but rather fixed (as proposed by @mountrcg ) . In going with that, the slider would need to make a reappearance here for the TT edit view.

@dsnallfot
Copy link
Contributor Author

Ok. Sounds reasonable for consistency. Should be doable

@mountrcg
Copy link
Contributor

@dsnallfot can I make a proposal for the slider, which will not be based on changing HBT with slider as currently, but changing the ratio / insulin percentage as also depicted on the slider.
refer to: mountrcg@7974cc2

@mountrcg
Copy link
Contributor

What would be great to save HBT in settings instead of pushing it as an extra variable to oref. I don't like this setup as it basically contradicts what is written in a preferences. In prefs currently the 160mg/dL resides and for an adv. TT oref gets a second HBT value that overrides the current setting. Not so good. What do you guys think?

@dnzxy
Copy link
Contributor

dnzxy commented May 26, 2024

Please do @mountrcg but in a separate PR. Daniel’s PR here can introduce the editability of presets and slightly change deletion behavior (alert, swipe action), but leaving logic as-is. You can fix / propose a change around the slider either in #154 or in a new PR 😊 let’s please not scopes here.

- Fix mgdl->mmol rounding when using experimental slider
- Also rearrange sliders below % text in experimental view (Reusing @dnzxy ols iAPS PR 321)
- Reset all changed values/sliders to 0 if cancelling in edit view
. Additional commit regarding experimental slider in edit view  upcoming soon in part 2
- Experimental slider now available also in editPresetPopover view
- All conversion mgdl/mmol tested and seems to work correct
- You can switch between normal and experimental view freely while contemplating your edit
- Additional features and UI redesigned excluded from scope
@dsnallfot
Copy link
Contributor Author

dsnallfot commented May 26, 2024

Demo with latest (last?) changes.
Experimental slider now available in edit mode

Video

2024-05-27_01-03-08.mp4

EDIT: Will also fix sliders % populating in edit view based on set targetsFIxed. Video demo updated

- when entering edit mode. This way you can choose "normal" target/duration entries or use the slider to do your edits
Copy link
Contributor

@bjornoleh bjornoleh left a comment

Choose a reason for hiding this comment

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

Tested on a phone, works as expected.

@bjornoleh bjornoleh linked an issue May 27, 2024 that may be closed by this pull request
Copy link
Contributor

@aug0211 aug0211 left a comment

Choose a reason for hiding this comment

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

Testing on physical device for 24h - looks good!

Copy link
Contributor

@dnzxy dnzxy left a comment

Choose a reason for hiding this comment

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

Tested on simulator device, looks good and works as expected. Very minor comment on a single variable name; I‘m okay with moving forward as-is though 😊

@Sjoerd-Bo3 Sjoerd-Bo3 self-requested a review May 30, 2024 12:21
Copy link
Contributor

@Sjoerd-Bo3 Sjoerd-Bo3 left a comment

Choose a reason for hiding this comment

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

Tested this with a live device for several days, edited some TT's during this period. And only found a thing with Experimental Setting, but that was due to my understanding of that. Using it a lot and works great.

@dnzxy
Copy link
Contributor

dnzxy commented May 30, 2024

Merging with 4 approvals.

@dnzxy dnzxy merged commit 5310235 into nightscout:dev May 30, 2024
1 check passed
@dnzxy dnzxy mentioned this pull request May 31, 2024
@dsnallfot dsnallfot deleted the dev-edit-tt branch June 16, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow Editing of Existing Profile Overrides
7 participants