-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Add non-pump insulin dialog #271
Add non-pump insulin dialog #271
Conversation
* New entry dialog to add non-pump insulin in history view * Removes `Add bolus without actually bolusing` functionality from the bolus entry dialog * New dialog adds ability to back-date insulin entries * Backdating is limited to past dates up to current date
|
Hi @Jon-b-m , thanks for the feedback. I'll reply one by one.
I have to strongly disagree with this notion due to various reasons. I'll list them below.
Let's please listen to our user base and to the feedback they have provided us with.
I really like the button placement as a first "header" row, as you had already suggested for PR #272 and will add this accordingly 🤓 The future entry filtering was dropped from the initial PR #236 , because it is giving issues with deleting entries (index offset) and because we had agreed, that FPU entries will be gone once we can refactor iAPS to make use of a fat-and-protein model and record meals rather than carbohydrates and carb-equivalents for fat and protein. Do you still think it's a valid addition? If so, I can look into it again 👍 |
@dnzxy @Jon-b-m I'm curious if this idea is acceptable as hybrid solution:
Done. Everything is now: Would that be workable/acceptable for everyone and solve the stalemate? Then we have everything in one place (insulin button), BUT we still have it clearly separated from each other; each with their own benefits as outlined above/as requested by the users to be clear like this. -edit- Modified "manual" to "external", because "manual" is already in use for when you "manually" add a bolus in the interface compared to SMB's ("automatic") and such. |
Yes, I like this idea, but I also have another solution in mind, a simpler. I will post here soon. |
I can work with this idea, too. If you agree it’s something you will accept, I‘ll start on it. |
I can agree about the danger of inadvertently adding external instead of new directly to pump. With this in mind I will accept change of this button to the history list. This way it's (more) coherent with the current dev glucose list. The "Display Future" here changes to "Hide Future" when displaying future. Hiding future is selected as default in my working test here. As you can see the amount of insulin is to the very right, to improve readability and to make the external insulin button more intuitive. |
Okay, I can adjust my PR accordingly and update the code. So we will keep with the dialog. I don’t think reshuffling the layout like this makes the info easily graspable. I think the layout proposed in #273 makes sense in this regard. Give me the chance to rework this please and I will ping you with the changes ready. |
* Change button style and add a header row to list * Add conditional filtering for future entries * Add filter button that changes text
I don't like that row you suggested. it looks sort of half baked compared to my suggestion. |
I have swipe to delete and the hide and show future entries already implemented in above suggestion. So you'll need to "compete" with this. |
Swipe to delete is part of PR #273. You have asked that I split the big PR up in as small parts as possible. This only PR only adds the new dialog and now the header row. The other fixes deletion. The row is the same to yours for about 98% of it. I used other icons (was not able to find that booklet icon you used for „Display Future“ in SFSymbols) and I use the same font size and (+) icon for the elements as are currently in dev for the glucose tab. It’s just consistent. If we merge #273, I will pull in dev to this PR, update it, then it will hold swipe on delete. |
it's all in the details. It just doesn't look good for a number of reasons. I used the Image(systemName: "book.pages") and for the insulin I used symbol "syringe". I used caption for text, to not distract from the import part, the data table entries, as it's implemented in glucose table in dev. With text in .secondary style, same as dev. |
Just Add and the syringe look much better and cleaner |
Are you using latest Xcode? |
You need to update your symbols app |
Added ✅ although I cannot verify. Running Xcode 15.0.1 here on macOS Ventura 13.6 which cannot show that in Xcode and I seem to be running latest Symbol Explorer app for Ventura, but that icon is new and part of Sonoma. Will update now, I trust you that it works, so pushed anyways 👍 |
@Jon-b-m I updated to latest macOS Sonoma and the icon still did not show. I then realized that the "issue" lies with iOS 17 vs. iOS 16 and earlier. I am always building to an iOS 16.4 iPhone 14 (to verify backward compatibility of the things we add), and it seems like the new icon So honestly, we cannot use it. Please let me know, what icon to change it for. I suggest using the logic that I initially had added, with the calendars with varying (-) and (+) symbols. Those icons are
|
* Invert default to false (Jon initially said this to be default) * Add comment to describe default * Fix condition for treatment list data population
Sorry. In that case use your current calendar option. |
Updated 👍 |
Text, Labels, Section etc don't need LocalizedString wrapper. |
* Removed unnecessary NSLocalizedString wrappers * Remove obsolete `.alert()` that was blocking FPU and carb deletion alert
change when to display warning message to: |
Add bolus without actually bolusing
functionality from the bolus entry dialogThis feature was inititally proposed through PR #236. Please see that PR for user feedback (about a dozen people have tested this) and test feedback. Many users requests a button with text and icon, so I added it back (it was removed from 236).