Color selection for Contact Image #349
Conversation
Added color picker
|
Thank you for your submission. All PR for LoopFollow should be against dev branch. Each PR will be tested/approved before merge to dev. The timing of the subsequent release to main depends on circumstances. |
|
Thanks @marionbarker. I wasn't sure if I should submit to Main or Dev. Should I redo this for the Dev branch? |
|
Sorry. I should have been more clear. I already changed the target branch to dev for you. (Anyone who created a PR can also tap edit and change the target branch.) I was able to do it for you because I have write permission for this repo. We are always happy to have new contributors, and sometimes make suggestions for future work. |
| @@ -68,10 +83,10 @@ class ContactImageUpdater { | |||
|
|
|||
| let maxFontSize: CGFloat = extra.isEmpty ? 200 : 160 | |||
| let fontSize = maxFontSize - CGFloat(bgValue.count * 15) | |||
|
|
|||
There was a problem hiding this comment.
Unintended whitespace change.
| @@ -1,4 +1,4 @@ | |||
| // | |||
| // | |||
There was a problem hiding this comment.
Unintended whitespace change.
| @@ -33,6 +33,7 @@ struct ContactSettingsView: View { | |||
| requestContactAccess() | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Unintended whitespace change.
|
Hi! Thanks for the contribution—it’s much appreciated! I made comments on the code regarding some unintended whitespace changes. It would be great to clean those up to keep the diff minimal and focused on the functional changes. I also noticed that some of the colors, for example the purple and blue, appear quite dark and are a bit hard to read (at least on my watch and with my eyes ;) ). Is this something you’ve noticed as well? Perhaps we could tweak the colors to improve readability. I’d like to hear your thoughts on this. Other than that, everything looks great! The code works as expected, and it aligns well with the patterns in the surrounding code. Nice work! 👍 |
|
Hi @bjorkert - I've finished working on the updates to use additional contact cards. This should allow for easier to read BGs including trend and delta on the Apple Watch. Could you please take a look and provide your feedback? In the Contact Settings, there's now options to select background and text color along with three options for showing the trend arrow and/or the delta. Both can be "Off" or "Separate" but only one can be "Include" where it combines the BG and the trend/delta. This operates just like the previous version except there are now three contact cards that users can choose to add to their watch face: LoopFollow - BG, - Trend, and - Delta 
Please let me know what you think! |
|
Hi! Thanks for the update! I'll try to look into this soon! |
|
mmol values should always be displayed with a decimal. Here is an example with a zero delta, should be +0,0 |
|
The delta is a bit too big with mmol, it should not "touch" the circle. |
|
I think there should be a short description for the separate contact cards, so the user know that another contact card should be selected? |
|
There is a merge conflict, but that is easily solvable in this case. Other than mentioned above, it looks good. I will use the delta in a separate card myself. |
|
Thanks for the feedback @bjorkert! I'll make these changes in the next few days. |
|
@mikebash You are still working on this, right? |
|
Hi @bjorkert - yes this is still on my to do list. Apologies for being slow - work has been especially busy. Adding the description and fixing the size of the delta will be straight forward. Forcing the mmol values to always be displayed with a decimal, shouldn't be tough. That said, I'm not totally sure where to apply it because I'm not sure why it's correct when you add the delta as part of the BG contact card but it's incorrect when it's on a separate contact card. If you have any quick tips off the top of your head on how to solve that easily, I wouldn't mind the help. Otherwise, I'll dig through the code to see where it's falling apart. Thank you! |
|
No, worries, just checking in :) I'll see if I can help out later on! |
|
@bjorkert I made the updates. Please take a look and let me know what you think. I'm wondering if we should make the vertical y value for drawing the numbers dynamic based on the font size. The numbers seem to be sitting a little high in the circles. Note that I'm still running this branch off 2.2.8 which is why (I think) the plus sign doesn't show up in the delta.
|
|
I agree, they are a bit high up, that’s unnecessary when there’s no other value displayed in the same box. Otherwise, it looks great! I can help you merge dev into your branch to resolve the merge conflicts. Just let me know when you take a break from coding, and I’ll take care of it and push the changes to your branch - when creating a PR on GitHub, there’s a setting that allows maintainers of the repo you’re submitting to edit your branch. |
|
I'm not really sure why the values are sitting too high. I stared at the code yesterday trying to figure out what is different from the original code and I'm not seeing it. Perhaps with fresh eyes I'll see it. Given our time zone differences (I'm in Chicago), you can safely trust that I am not coding until after midnight your time (unless it's the weekend) so if you wanted to push those changes, go for it. Thanks for the help along the way. It's been fun to work on. |
|
@bjorkert thanks as always for your patience. I adjusted the vertical orientation on the numbers to better center them. At this point, I think this work is complete unless you have any additional feedback. Thanks! |
Added a color selector in the Contact settings so individuals can choose different colors based on their watch faces and their preferences. Also should allow individuals with multiple loopers to be able to assign a different color to each. I have tested this on my phone and watch but before merging into the main branch, this new code will benefit from additional code review and/or testing. Thank you!