nightscout / Trio

MIT License
46 stars 130 forks source link

Add Tidepool navigation view (fix missing chevron) #227

Closed dsnallfot closed 1 month ago

dsnallfot commented 1 month ago

Demo of PR implementation:

https://github.com/nightscout/Trio/assets/72826201/28b50e36-f08b-4486-9623-b58257a0e6f0

Settings navigation before change (missing chevron) image0

marionbarker commented 1 month ago

Suggest using Tidepool (instead of TidePool) for variable, function and file names.

dsnallfot commented 1 month ago

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) This PR increased the usage to 49 times in 9 files (excluding filenames and project.pbxproj) due to the addition of TidePoolStartView file and moving existing code from SettingsRootView to TidePoolStartView.

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 :/

TidePool TidePool TidePool
Skärmavbild 2024-05-22 kl  13 18 04 Skärmavbild 2024-05-22 kl  13 18 39 Skärmavbild 2024-05-22 kl  13 18 56
MikePlante1 commented 1 month ago

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

dsnallfot commented 1 month ago

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)

aug0211 commented 1 month ago

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.

marionbarker commented 1 month ago

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.

dsnallfot commented 1 month ago

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

dsnallfot commented 1 month ago

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.

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

dsnallfot commented 1 month ago

@MikePlante1 I cannot add reviewers for the PR myself, could you please help me with that?

bjornoleh commented 1 month ago

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 modified: Trio.xcworkspace/xcshareddata/swiftpm/Package.resolved when checking out this branch. Others seing this?

dsnallfot commented 1 month ago

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 modified: Trio.xcworkspace/xcshareddata/swiftpm/Package.resolved when checking out this branch. Others seing this?

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

Skärmavbild 2024-05-22 kl  23 58 23
dsnallfot commented 1 month ago

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 modified: Trio.xcworkspace/xcshareddata/swiftpm/Package.resolved when checking out this branch. Others seing this?

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?

https://github.com/nightscout/Trio/assets/72826201/d2bb9aea-5826-4eb9-9455-2a81562d43d2

bjornoleh commented 1 month ago

It happens every time on first tap and ok on there second. Not sure if I had previously set up TP on this build

dsnallfot commented 1 month ago

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)

bjornoleh commented 1 month ago

I did build twice by the way, with a build of dev in between. Same result for the PR branch builds.

dsnallfot commented 1 month ago

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. Will keep you posted!

dsnallfot commented 1 month ago

@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 😊

bjornoleh commented 1 month ago

@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.

dnzxy commented 1 month ago

LGTM. Great work, Daniel and wonderful discussion here. Thanks everyone.

Merging with 2 approving requests.