MBL-1039: Refactor LoadingBarButtonItem to take a closure instead of a binding for save actions #1881
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -74,17 +74,12 @@ struct ReportProjectFormView: View { | |||
| viewModel.inputs.viewDidLoad() | |||
| } | |||
| .onReceive(viewModel.$bannerMessage) { newValue in | |||
| showLoading = false | |||
There was a problem hiding this comment.
This is moved up in the view model
…a binding for save actions
amy-at-kickstarter
force-pushed
the
refactor/adyer/mb-1039
branch
from
5769716
to
46a835a
Compare
| @@ -243,6 +244,10 @@ public final class ChangeEmailViewModelSwiftUIIntegrationTest: ChangeEmailViewMo | |||
| self.viewDidLoadProperty.value = () | |||
| } | |||
|
|
|||
| public func didTapSaveButton() { | |||
| self.saveTriggered.send(true) | |||
There was a problem hiding this comment.
Refactoring this to be a stream of voids would be a slightly bigger project; leaving that for another day.
| // An API error happens. | ||
| // We need to catch this up here in flatMap, instead of in sink, | ||
| // because we don't want an API failure to cancel this pipeline. | ||
| // If the pipeline gets canceled, you can't re-submit after a failure. |
There was a problem hiding this comment.
This was something interesting I noticed while testing. Any error in a pipeline cancels/finishes the entire pipeline.
There was a problem hiding this comment.
Good find! That's useful to know!
Codecov Report
@@ Coverage Diff @@
## main #1881 +/- ##
==========================================
- Coverage 83.83% 83.83% -0.01%
==========================================
Files 1224 1224
Lines 111347 111349 +2
Branches 29599 29601 +2
==========================================
- Hits 93350 93349 -1
- Misses 16984 16989 +5
+ Partials 1013 1011 -2
... and 1 file with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
ifosli
approved these changes
Nov 8, 2023
| // An API error happens. | ||
| // We need to catch this up here in flatMap, instead of in sink, | ||
| // because we don't want an API failure to cancel this pipeline. | ||
| // If the pipeline gets canceled, you can't re-submit after a failure. |
There was a problem hiding this comment.
Good find! That's useful to know!
ifosli
pushed a commit
that referenced
this pull request
Nov 13, 2023
…a binding for save actions (#1881)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📲 What
This refactors
LoadingBarButtonItemto take a closure.🤔 Why
This pattern more closely follows how Apple handles save actions (e.g. in the
Buttonclass), which will make our code a little bit more canonical.✅ Acceptance criteria