nightscout / Trio

MIT License
46 stars 130 forks source link

Enable editing of preset profile overrides #235

Closed dsnallfot closed 1 month ago

dsnallfot commented 1 month ago

Suggested solution for issue #234

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 Settings summary before saving
Simulator Screenshot - iPhone 13 mini+ - 2024-05-25 at 01 55 40 Simulator Screenshot - iPhone 13 mini+ - 2024-05-25 at 01 56 39
dsnallfot commented 1 month ago

Short video demo

https://github.com/nightscout/Trio/assets/72826201/c66fe66d-259c-4dd8-a9a2-d647594d8767

dnzxy commented 1 month ago

The UI on this really is not intuitive. A swipe action implies that the selected element will be edited and that an action will prompt past the point of action, here the swipe. A user would expect to swipe and tap the edit button and then be forwarded to an editing mask or something similar where the data of the preset is prefilled and the you made in the „root“ view are then done. Here, the edit action actually saves data that was edited before.

I like that this gets some love, but also want to urge that we held of on any UI work past 1.0.0, and well, this is UI work.

dsnallfot commented 1 month ago

I'd say it's more UX than UI work to be able to edit stuff. But I totally agree. To make this intuitive the settings and slider should get moved into the popup.

What's even less intuitive is not being able to edit an entry at all. To have to delete it, and then recreate it with new settings and the same name (but other id).

But I guess you already have implemented all this functionality in your upcoming UI rework (that I have not seen myself) that will take place after 1.0?

Well. Then we can close both those PRs for just mine and Auggies personal use for now and wait until the big UI boom after 1.0 solves all outstanding UI/UX-issues. 😊

dnzxy commented 1 month ago

You are totally right, it’s UX, I just always see UI/UX as a whole. Adding UX usually also involved changing the UI (here introduce a new view and swipe actions).

Also fully agree not being able to delete it at all is unintuitive, yes. If this can be made so that you trigger a subsequent edit mask or the current course of action is more transparent, e.g. Save or select preset to edit? How about that?

We haven’t tackled the profiles section with the specific edit functionality you have proposed here, but just moved things and are mostly working on persistence layer.

Can we leave this open as a draft and move the idea of this over once we are at that point? I really want to get something like this in, but we need to make use of design elements and HCI as intended and here the swipe action ought to trigger an edit view view or a subsequent action 😊

dsnallfot commented 1 month ago

Closed for now. To be revisited in the future after 1.0 release.

aug0211 commented 1 month ago

Thanks Daniel, I’m definitely still using in my personal fork. Not being able to edit overrides/temp targets has been a nightmare for me with, creating an huge amount of toil.

dsnallfot commented 1 month ago

Reopen and put in the "idea box" until after 1.0 release. Will continue to push a few commits to this feature branch until then. (To move the settings adjustment inside the popup for instance)

avouspierre commented 1 month ago

Amazing @dsnallfot ! I proposed some changes to be more in editing mode like UX Apple. The idea is to start by choosing "editing" mode, to modify all the value and to save after. I also added a cancel button.

The example is here : Video Image
CleanShot 2024-05-25 at 14 48 41 CleanShot 2024-05-25 at 14 51 45@2x

I currently do the update in the PR #219 but depends what the trio's community prefer !

dsnallfot commented 1 month ago

That's a beautiful solution Pierre! Much better than my quick draft.

I knew you were working with overrides, but not this edit things (I actually checked your PR to make sure there wasn't that much conflicts in override state and root.

Very nice!

dnzxy commented 1 month ago

@avouspierre ‘s way of editing is great and uses the swipe action just like I would expect it to in iOS. Great job!

Edit to add; could we split that from your current override PR and add it here? Would make the scope smaller and then it’s just the edit feature 😊

dsnallfot commented 1 month ago

I think Pierres implementation is more promising, advanced (and prettier), but I at least in this draft 2.0 I fixed the backwards workflow by moving the settings inside the edit popup view

Edit and delete Edit pop up
Simulator Screenshot - iPhone 13 mini+ - 2024-05-25 at 21 53 20 Simulator Screenshot - iPhone 13 mini+ - 2024-05-25 at 21 52 53

Video

https://github.com/nightscout/Trio/assets/72826201/98689c66-094a-4cb9-8333-f1789446c278

dnzxy commented 1 month ago

I like the latest proposal by you, @dsnallfot , more than the version @avouspierre has proposed.

Pierre's version is equally as nice as yours, as both of you use the swipe action to toggle a subsequent action. However, your version opens a sub-view, pre-populates the values for the various inputs and options, asks the user to change something, then the user saves, view disappears, finished. It's a very nicely guided, "holding the user's hand" type approach.

Pierre's approach loads the preset values into the inputs / options in the root view, making it harder to grasp that "something" has happened or changed.

I must admit, I prefer your option and I'd even go as far as proposing that the view you use to edit a preset should actually also be the view to add a preset and/or start a non-preset profile override. So ideally the profile override root view would consist of a big Add or enact button, or a big [+] button, and the preset list. If there are no profiles stored/created yet, the list could show a little hint or simply display "No presets found. Proceed to create preset or enact profile override." This would also shrink the entire root view, getting rid of the scroll view to get to the start / add / cancel buttons. Imho, whenever a user needs to scroll, the view has potential to be refined so that they don't have to (unless it's settings, long text, lots of data displayed etc.)

Either way, both proposals are a huge improvement over what is currently there.

bjornoleh commented 1 month ago

Looks good. Would it be possible to add a confirmation before deleting? It’s pretty easy to accidentally delete when the intention was to edit?

dsnallfot commented 1 month ago

Looks good. Would it be possible to add a confirmation before deleting? It’s pretty easy to accidentally delete when the intention was to edit?

Of course possible, I use it in my personal build. BUT there is an annoying UI glitch with the row disappearing and quickly reappearing when the alert is triggered. I can live with that in a personal build, but not good enough for the official fork.

If we feel that an alert is important to add, I can dig further in this later on (remember that Dan or someone fixed the exact same glitch when deleting in datable (history)

Demo of my glitchy personal implementation as of right now (Sorry for Swedish text, didn't have my Norwegian version up n running right now)

https://github.com/nightscout/Trio/assets/72826201/682a6aff-1adf-4ef8-90d4-1d148cb25580

dsnallfot commented 1 month ago

Nevermind! Figured it out. Just needed to set the swipebutton to 'role .none' instead of '.destructive'. See new demo.

Do we want this alert?

New glitchfree demo

https://github.com/nightscout/Trio/assets/72826201/1c6954bf-07a4-4a29-b0de-5cdceddce323

dsnallfot commented 1 month ago

And alerts before deletion is now in. I figured its almost a no brainer to include it, and I'm pretty satisfied with this implementation (it shows the profile name you have swiped on for extra clarity when deciding to delete or not)

https://github.com/nightscout/Trio/assets/72826201/0a5ee2c5-4bc5-4682-9ad1-46581348b689

dnzxy commented 1 month ago

Was just about to comment that the issue is with the default onDelete modifier and can be fixed - but Daniel already figured it out 😅 It looks very similar to how we do it in the DataTable and that works great my

I really like this feature now, great job!

dnzxy commented 1 month ago

Revisiting this PR today and looking again at my proposal from yesterday (quoted below)

I must admit, I prefer your option and I'd even go as far as proposing that the view you use to edit a preset should actually also be the view to add a preset and/or start a non-preset profile override. So ideally the profile override root view would consist of a big Add or enact button, or a big [+] button, and the preset list. If there are no profiles stored/created yet, the list could show a little hint or simply display "No presets found. Proceed to create preset or enact profile override." This would also shrink the entire root view, getting rid of the scroll view to get to the start / add / cancel buttons. Imho, whenever a user needs to scroll, the view has potential to be refined so that they don't have to (unless it's settings, long text, lots of data displayed etc.)

I think we should make that a second PR. This PR looks very feature-complete (full editability, deletion with alert for consistency with history view) and thus should be tested, approved and merged.

In a subsequent PR we can build on this and make the creation / enactment of a profile along the lines of my proposal - if that is something we want to move forward with. I‘d vote yes (naturally, haha), as it’s an iOS-esque change to creating something and also similar behavior to how basal profiles and other types of "listed data" is created in the app.

Further more, the changes from this PR would also need to be ported over to #236 so that we have consistency there (already done by Daniel ✅).

I do have a second follow-up proposal about profiles and TTs, but let’s discuss that when the time comes 😊🙏

avouspierre commented 1 month ago

I checkout the code and test the new functionalities. All functions "add,edit, delete" preset profile are OK.

Some improvements for maintenance POV @dsnallfot :

Functionaly, a proposal : perhaps not apply the edited profile as a current profile ? Example with the video :

CleanShot 2024-05-26 at 19 50 12

dsnallfot commented 1 month ago

Thanks Pierre! Will go through all your suggestions and try to adjust accordingly.

dsnallfot commented 1 month ago

Will soon commit all changes based on all your input regarding both refactoring and improved functionality @avouspierre , thanks again for great feedback!

And below is a demo of the improved functionality regarding what pre filled settings are used in the view.

  1. I've made .onDisappear from the editPresetPopover back to root view behave the same way as .onAppear in root view when coming from home screen, that is:
    • If ongoing override, populate settings with the ongoing override setting.
    • If no ongoing override, populate settings with user defaults
  2. And when entering edit by selecting edit on any override -> populate the settings in edit mode with that selected overrides settings as starting point for the edit

Do this look ok?

https://github.com/nightscout/Trio/assets/72826201/6bd189df-43cf-40a2-a555-68b1fb0bf512

dsnallfot commented 1 month ago

Now we are hopefully starting to see the finish line for this PR. Regarding refactoring and avoiding duplicate code (note: there are som duplicate code in state model since before that I didn't touch) - and for new functions, I've tried the best I could to make the code ok and taken it as far as I can with making it pretty and maintainable.

If any more adjustments regarding maintenance and duplicates/code efficiency is needed, I really need help from anyone of you guys.

Well. Every change and improvement needed (so far) is committed in the PR now, and you could start testing when it suits you.

And the screenshot below is the "one more thing commit" that I almost forgot to add, the slider & percentage order switcharoo @dnzxy made in iAPS after 2.3.3 😊

Simulator Screenshot - iPhone 13 mini+ - 2024-05-27 at 09 14 17

dnzxy commented 1 month ago

Looking superb. Great job.

dnzxy commented 1 month ago

Merging with 4 approvals.