Add haptics for merge results #2690
Conversation
Update master branch
Huddie
left a comment
Huddie
left a comment
There was a problem hiding this comment.
Looks good. We're you able to test this or just assuming it should work properly?
Huddie
added
🏁 ready to pull
|
I tested this feature on a test repository before sending PR. |
BasThomas
left a comment
BasThomas
left a comment
There was a problem hiding this comment.
Nice, two tiny things!
| number: Int, | ||
| type: IssueMergeType, | ||
| error: @escaping () -> Void | ||
| completionHandler: @escaping (Bool) -> Void |
There was a problem hiding this comment.
Can we add a label to this tuple, like @escaping (success: Bool) -> Void? That makes it a bit easier to understand what the Bool is referring to.
There was a problem hiding this comment.
Sorry for bad english 😅
Do you want to fix it yourself or so that I fix it?
There was a problem hiding this comment.
If you want to fix it, go for it!
| error: { [weak self] in | ||
| self?.loading = false | ||
| self?.update(animated: true) | ||
| completionHandler: { [weak self] isSuccessMerge in |
There was a problem hiding this comment.
Super-nit: isSuccessfulMerge?
There was a problem hiding this comment.
Yes, it would be better
akarataev
force-pushed
the
2668-haptics-for-merge-results
branch
from
ff6857b to
eb8d2f2
Compare
| number: Int, | ||
| type: IssueMergeType, | ||
| error: @escaping () -> Void | ||
| completionHandler: @escaping (_ success: Bool) -> Void |
There was a problem hiding this comment.
Why do you ignore it still? That wouldn't help to understand it at the call site 😄
There was a problem hiding this comment.
@BasThomas functional type can't have a argument label. If you ask about it.
3 participants
I've implemented a haptic handler for a successful and unsuccessful merge results.