-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: add in-app event listener #211
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
c76db13
feat: add event listener for gist events
levibostian 369e740
rename messageOpened to messageShown
levibostian 51f9f3f
add back initialize without event listener
levibostian 5ef489a
deliveryid can be null for in-app message
levibostian c12fc1a
attempt to fix lint on ci
levibostian b48ceee
fix ci lint
levibostian 1d3c023
Merge branch 'develop' into levi/gist-event-listeners
levibostian 50c4283
remove parameters from listener not needed
levibostian d7f188c
Merge branch 'develop' into levi/gist-event-listeners
levibostian da93b71
test when sdk not initialized yet
levibostian a371201
add note [skip ci]
levibostian c7897d0
fix: gist set user token incase identifier exists (#253)
Shahroz16 6b9554b
docs: add docs from pr review
levibostian 40fa856
refactor: remove duplicate code for initialize
levibostian 1b24c7c
Merge branch 'develop' into levi/gist-event-listeners
levibostian f0e429c
add resolved package
levibostian cbfff62
update from code review
levibostian File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import Common | ||
import Foundation | ||
import Gist | ||
|
||
public protocol InAppEventListener: AutoMockable { | ||
func messageShown(message: InAppMessage) | ||
func messageDismissed(message: InAppMessage) | ||
func errorWithMessage(message: InAppMessage) | ||
func messageActionTaken(message: InAppMessage, action: String, name: String) | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import Foundation | ||
import Gist | ||
|
||
typealias GistMessage = Message | ||
|
||
public struct InAppMessage: Equatable { | ||
public let messageId: String | ||
public let deliveryId: String? // (Currently taken from Gist's campaignId property). Can be nil when sending test | ||
// in-app messages | ||
|
||
internal init(gistMessage: GistMessage) { | ||
self.messageId = gistMessage.messageId | ||
self.deliveryId = gistMessage.gistProperties.campaignId | ||
} | ||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea but if all its doing is logging a message why are we calling it,
sdkNotInitializedAlert
because maybe we might have more alerts that could utilize this method?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about making the naming more generic. However, I found some cons with that approach.
In the function body, you see
"⚠️ \(message)", level: .error
. So, we are prefixing all log messages with⚠️
and we are using the log level of error.If we were to rename this function to something more generic:
sdkNotInitializedAlert()
will now have copy/pasted code in it.sdkNotInitializedAlert
grows. Currently, the only edge case I can consider needing logging without a DI graph is when the SDK is not initialized.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, not a blocker to PR, but maybe an idea.
maybe, have an
icon enum
type for where we will use⚠️ message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am leaning towards the current implementation of
sdkNotInitializedAlert
becauselogMessage
doesn't tell the developer when to use that function to log a message and when to use theLogger
that's inside of the DI graph. A function name such assdkNotInitializedAlert
suggests that this only be used when the SDK is not initialized. Otherwise, use the logger in the DI graph.sdkNotInitializedAlert
's implementation is where the icon and log level are encapsulated into 1 place in the code. If we were to replace it with:logMessage(message: "foo", level: .error, icon: .warning)
, it would no longer be DRY.