Add Tidepool navigation view (fix missing chevron) #227
Conversation
- One more click to reach the service, but - Added chevron in settings list (for consistency) - Added room for information/explainations of the setting in header/footer in the new view - First draft to build on (maybe: rephrase header/footer, add connection status in navigation view, other relevant info etc)
- and remove uneccessary header
|
Suggest using Tidepool (instead of TidePool) for variable, function and file names. |
Thats a relevant suggestion. However before this PR the incorrect "TidePool..." instead of "Tidepool..." was used 48 times in 8 files (excluding filenames and project.pbxproj) I changed all user visible TidePool text to Tidepool in the PR. I could change TidePool to Tidepool in all added code in the PR, but then the new code and the old code is inconsistent. Or I could of course change all 49 TidePool entries but then all those files below are impacted. Not sure what's the best move here :/
|
|
We’ll have to do some tests to make sure it doesn’t affect anything, ofc, but I think we should change all the Ps to lowercase |
|
Ok Mike. We might as well do it now rather than wait for later. Will update the PR with this later today (increased risk of merge conflicts with other ongoing PRs/issues of course since more files will be changed) |
|
Can I suggest an alternate route? View these as two separate issues. Accept and merge this PR as is. If I understand correctly, this PR in current form is consistent with the current state, matching the other 48 occurrences, and increases it by 1 (~2%). We could merge this, knowing that it follows the standard in all other locations of Trio, and follow up with a separate PR to address the capitalization issue. |
-and also tidePool* to tidepool*
|
Any change to a project.pbxproj is problematic. At least update the filename. Or do the rename all at once and we commit to test, approve and merge ASAP. Then all other PR can merge dev if needed. |
project.pbxproj names also updated with lowercases now (Xcode does that automagically when filenames are changed). I have test built successfully, but it doesn't hurt that we test this thoroughly |
Sorry Auggie. I pushed the commit before I browser refreshed this GH issue and saw your comment so I didn't consider it before changing |
|
@MikePlante1 I cannot add reviewers for the PR myself, could you please help me with that? |
dnzxy
requested review from
Sjoerd-Bo3,
marionbarker,
dnzxy,
bjornoleh and
MikePlante1
- Footer text in 2 elements - spelling error fix and Connect to Tidepool from ontapgesture text -> Action button with blue text for consistency
|
When tapping the Connect to Tidepool button, I am being returned to the Tidepool settings root the first time I tap it, but the second time I am taken to the Tidepool login screen. The same happens each time I escape the Tidepool settings and try again. I am also getting |
Hm. I have not seen that error you're describing with need to double tap. Will investigate. (No code affecting this are changed in the PR) And the other thing: I don't know exactly why this occurs, but there is an uncommitted Package.resolved change in my Xcode clone, see attached screenshot. I did not upload it to the PR since it isn't anything that should be altered in this PR. But if someone understands this better, please inform me |
Looking at the code, the thing that happens when you press the button is that ´state.setupTidepool = true´. The default state is false. Maybe you exited a previous build when the state was true? And therefor the button didn't trigger the sheet? I cannot reproduce it. Could you try to rebuild and see if it persists? Simulator.Screen.Recording.-.iPhone.14.-.2024-05-23.at.00.15.06.mp4 |
|
It happens every time on first tap and ok on there second. Not sure if I had previously set up TP on this build |
I will try to build to my physical simulator iPhone and test that way also (have just run it in Xcode sim) |
|
I did build twice by the way, with a build of dev in between. Same result for the PR branch builds. |
Ok. I hope someone else can comment as soon as they've tested so we know if this affects all builds or if there are some special circumstances that triggers this. edit 1: FYI: While waiting for TestFlight to finish I built on 3 different clean iPhone simulators (with no previous Trio settings). All worked as expected - no double tapping needed... edit 2: Also tested to login on clean dev sim build. Then build pr build over. Credentials was not saved (needed to login again). but no double tapping needed. Edit 3: Just built on real phone. I see the exact same issue that you described. Good! (Or not good). But then I know I need to investigate how the bools behave. |
- Sheet needed to be moved outside Form (Swift curiosity)
|
@bjornoleh. After intense googling and code debugging I found some other devs discussing this exact peculiar sheet behavior that doesn't affect simulators but shows up on physical phone builds. (I could for instance see when adding some debug logging that the sheet always got triggered 3 times in a row the first time I opened the Tidepool navigation view). And all I had to do to resolve this bug was to move the sheet code 2 brackets down, outside the form. Now it should work as intended again (both on simulator and physical phone). If not, let me know 😊 |
Thanks, well done figuring out this ridiculous issue! Works as expected now when building to a phone. |
|
LGTM. Great work, Daniel and wonderful discussion here. Thanks everyone. Merging with 2 approving requests. |
Demo of PR implementation:
Simulator.Screen.Recording.-.iPhone.14.-.2024-05-22.at.10.12.47.mp4
Settings navigation before change (missing chevron)
