Add possibility to react to notification tap via new CRNotificationDelegate #26
Conversation
|
Hey, thanks for the PR! I like the idea, and I guess the intention is that you want to execute the same piece of code in the completion handler, without duplicating the code - is that the case? If so, there is nothing stopping you from simply passing in a function instead of writing the same block each time. One could simply just pass in the same function as the one executed in the delegate. Usually the purpose of the delegate is to handle some sort of value(s) passed to the function. Maybe the type, title and message could be passed along, so you can reach on the type of notification shown, whether it being a success, failure or something else. Is that something you could add? 馃檪 |
| @@ -164,6 +164,15 @@ public class CRNotificationView: UIView, CRNotification { | |||
| }) | |||
| } | |||
|
|
|||
| @objc internal func dismissNotificationOnTap() { | |||
There was a problem hiding this comment.
I think its fine to just reuse the current method dismissNotification and simply call the delegate, followed by the normal procedure. Otherwise you will have two different implementations of the dismiss functionality and one might not hide correctly 馃檪
Swift will just call the function on the delegate if it's there. There is no need to guard it.
Simply just write onClickDelegate?.onNotitificationTap() 馃檪
There was a problem hiding this comment.
as I wrote below, I'd like to handle tapping on the notification differently than just sliding it away. Maybe it should be renamed to dismissNotification and tapNotification? or expandNotification even though this might suggest some more functionality.
Thank you for the Swift tip, you are right!
| @@ -39,7 +39,7 @@ public class CRNotificationView: UIView, CRNotification { | |||
| }() | |||
|
|
|||
| private var completion: () -> () = {} | |||
|
|
|||
| public var onClickDelegate: CRNotificationDelegate? | |||
There was a problem hiding this comment.
Maybe this could just be called delegate? Then other behaviour might be added in the future, such as a swipe, manual hiding or even buttons below the text. As such we are no longer limited by the name. I was thinking to add two small reaction buttons at the bottom. Then that button can be forwarded here, too 馃檪
There was a problem hiding this comment.
yes, calling it delegate sounds good!
|
Hey, thank you for the quick reply! |
add parameters to delegate rename delegate remove guard statement
|
Sorry for the delay on the review, I've been a little busy. Looks good! 馃檪 |
|
If there is nothing more you'd like to do on this PR I will merge it 馃憤 |
|
Thank you! :) |
|
Its now in master as 1.2.1. Thanks for the contribution 馃殌 馃檪 |
I think this is a neat little functionality expanding the cool & simple to use CRNotification! :)
It also does not change the API so it is backwards compatible, only if needed, a delegate can be set. It is also very simple to comply with the delegate.
If you want a more fancy feature, one could also include the text (or other information) of the notification in the delegate call.
As this is my first pull request on GitHub, please take a look and let me know if I need to improve something or if you don't like the feature 馃槃