Skip to content
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 pump” button when pump has not been configured #184

Merged
merged 2 commits into from
May 18, 2024

Conversation

avouspierre
Copy link
Contributor

@avouspierre avouspierre commented May 14, 2024

The PR adds a “pump” icon and “add pump” label in the header of the main view.
The tap gesture open the view to select a pump.

Link with #171 to review.

Simulator Screenshot - iPhone 15 - 2024-05-14 at 17 14 04

The PR adds a “pump” icon and “add pump” label in the header of the main view.
bjornoleh
bjornoleh previously approved these changes May 14, 2024
Copy link
Contributor

@bjornoleh bjornoleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected during brief testing by adding and removing the sim or Dash pump driver on a non.looping phone. Thanks!

I did not inspect the code.

@bjornoleh bjornoleh requested a review from Sjoerd-Bo3 May 14, 2024 16:15
minor updates about code.
Copy link

sonarcloud bot commented May 16, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@bjornoleh
Copy link
Contributor

Do you plan on resolving the remaining 2 sonarcloud issues? If so I'll wait with reviewing :-)

@avouspierre
Copy link
Contributor Author

Do you plan on resolving the remaining 2 sonarcloud issues? If so I'll wait with reviewing :-)

That's the problem with a "minor" fix required to review again. I don't think to resolve the 2 sonarcloud issues because all the (old) code is based on the double or triple closures in swiftUI. Not sure this rule is very useful and improve the maintenance of the code.

@marionbarker
Copy link
Contributor

Code review - some of this is beyond my skill level, but all looks reasonable.

Testing:

Configuration:

  • SE phone, iOS 17.5, Trio dev branch (commit 56a3f87), no pump attached (graphic on left)
  • git apply trio-pr-184.patch` and rebuild (graphic on right)

pr-184-success

Copy link
Contributor

@bjornoleh bjornoleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still works as expected by testing on the phone. Merging after two approving reviews.

@bjornoleh bjornoleh merged commit c4f65c7 into nightscout:dev May 18, 2024
1 check passed
@MikePlante1 MikePlante1 mentioned this pull request May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature request]-“Add pump” button when pump has not been configured
3 participants