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

Implement a generic AsyncButton with support for throwing closures #8

Merged
merged 6 commits into from
Jul 13, 2023

Conversation

Supereg
Copy link
Member

@Supereg Supereg commented Jul 13, 2023

Implement a generic AsyncButton with support for throwing closures

♻️ Current situation & Problem

Currently, Spezi Views (nor SwiftUI) provide a way to easily manage buttons with asynchronous action closures. We have several use cases where a reusable button with an async (throwing) action closure would be helpful (e.g. SpeziAccount for Login/Signup/Password Reset buttons).

💡 Proposed solution

This PR introduces a the new, reusable component AsyncButton that allows to supply async and async throwing action closures. It handles disabling the button and rendering of a processing indicator automatically. To deal with thrown errors of the action closure, we reuse the ViewState abstraction already available in SpeziViews. This can easily be combined with the viewStateAlert(state:) modifier to display a simple error alert.

⚙️ Release Notes

  • Added a AsyncButton component for async and async throwing action closures.
  • Added the processingDebounceDuration EnvironmentKey for a generalized configuration of debounce durations.
  • Added the processingOverlay(isProcessing:overlay) modifier to easily replace a view with a processing indicator.

➕ Additional Information

Related PRs

This PR refines the AsyncDataEntrySubmitButton implementation currently used in the upcoming SpeziAccount PR StanfordSpezi/SpeziAccount#7 and moves it to SpeziViews.

Testing

Additional UI tests were added.

Reviewer Nudging

Ideally, have a look at the added UI tests and then at the AsyncButton implementation itself. It might be helpful to play around with the Previews of AsyncButton as well.

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@Supereg Supereg requested a review from PSchmiedmayer July 13, 2023 21:46
@Supereg
Copy link
Member Author

Supereg commented Jul 13, 2023

@PSchmiedmayer SpeziViews seems to have an outdated list of required status checks in the branch protection rule.

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Nice, this is a great addition! 🚀
Love the usage of environment values to configure the behaviour, really follows the trend of the latest SwiftUI APIs out there.

Only had a few smaller comments. Feel free to make the conversations as resolved when they are addressed and merge the PR after that 🚀

@PSchmiedmayer
Copy link
Member

Fixed the merge check requirements @Supereg 👍

@Supereg Supereg changed the title Implement a generic AsyncButton with support for a throwing closure Implement a generic AsyncButton with support for throwing closures Jul 13, 2023
@Supereg
Copy link
Member Author

Supereg commented Jul 13, 2023

Fixed the merge check requirements @Supereg 👍

The Upload Coverage Report action is still referencing the old one. If you could update that as well 😇

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #8 (11beb6d) into main (bae5330) will decrease coverage by 1.36%.
The diff coverage is 72.23%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
- Coverage   78.11%   76.74%   -1.36%     
==========================================
  Files          19       22       +3     
  Lines         612      748     +136     
==========================================
+ Hits          478      574      +96     
- Misses        134      174      +40     
Impacted Files Coverage Δ
...eziViews/Environment/DefaultErrorDescription.swift 100.00% <ø> (ø)
...Views/Environment/ProcessingDebounceDuration.swift 50.00% <50.00%> (ø)
Sources/SpeziViews/Views/Button/AsyncButton.swift 70.80% <70.80%> (ø)
...es/SpeziViews/ViewModifier/ProcessingOverlay.swift 76.48% <76.48%> (ø)
Sources/SpeziViews/Model/ViewState.swift 97.83% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bae5330...11beb6d. Read the comment docs.

@Supereg
Copy link
Member Author

Supereg commented Jul 13, 2023

@PSchmiedmayer Do you remember how we fixed the issue of a 0% code coverage 🙈

EDIT: nevermind. Codecov was still outdated.

@Supereg
Copy link
Member Author

Supereg commented Jul 13, 2023

Was there an easy way to exclude PreviewProviders from code coverage reporting?

@PSchmiedmayer
Copy link
Member

PSchmiedmayer commented Jul 13, 2023

Not really, that is an unfortunate thing that Xcode does not really support. I have tried a lot of things with a dedicated TEST build configuration in the Template App but this wasn't ideal either and results in large build times when switching between DEBUG and TEST builds ...
I can merge the PR like this.

Regarding code coverage: Sometimes Xcode is flaky with the test reporting. If that happens, I have made a good experience adding the target to the build scheme for tests (e.g. SpeziViews in this case):
Screenshot 2023-07-13 at 4 19 37 PM

And then explicitly adding it in the coverage report settings:
Screenshot 2023-07-13 at 4 20 04 PM

@Supereg
Copy link
Member Author

Supereg commented Jul 13, 2023

Thanks for the tipps regarding code coverage. feel free to merge 👍

@PSchmiedmayer PSchmiedmayer merged commit 10ae4b1 into main Jul 13, 2023
@PSchmiedmayer PSchmiedmayer deleted the feature/async-button branch July 13, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants