chore: get WebView for inline message and add to Inline View to display #717
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
This was referenced May 17, 2024
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.
|
levibostian
force-pushed
the
spr/main/025c7bf5
branch
from
f506cd0 to
cc11137
Compare
levibostian
force-pushed
the
spr/main/60639329
branch
from
930f336 to
7498c5d
Compare
levibostian
commented
May 17, 2024
There was a problem hiding this comment.
This is the main logic of this PR. MessageManager contains a lot of code that's common between inline and modal messages. ModalMessageManager contains code I pulled out of MessageManager that's only used for modal messages.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature-inline-inapp #717 +/- ##
=======================================================
Coverage ? 58.25%
=======================================================
Files ? 142
Lines ? 3953
Branches ? 0
=======================================================
Hits ? 2303
Misses ? 1650
Partials ? 0 ☔ View full report in Codecov by Sentry. |
levibostian
force-pushed
the
spr/main/60639329
branch
from
7498c5d to
3c6243d
Compare
levibostian
force-pushed
the
spr/main/025c7bf5
branch
from
cc11137 to
4a32afa
Compare
levibostian
commented
May 20, 2024
Comment on lines
-211
to
-222
| if route == currentMessage.messageId, !messageLoaded { | ||
| messageLoaded = true | ||
| if isMessageEmbed { | ||
| delegate?.messageShown(message: currentMessage) | ||
| } else { | ||
| if UIApplication.shared.applicationState == .active { | ||
| loadModalMessage() | ||
| } else { | ||
| Gist.shared.removeMessageManager(instanceId: currentMessage.instanceId) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is code that should only be called for modal messages. So I moved this block into the Modal manager subclass.
levibostian
commented
May 20, 2024
Comment on lines
-64
to
-70
| if let modalViewManager = modalViewManager { | ||
| modalViewManager.dismissModalView { [weak self] in | ||
| guard let self = self else { return } | ||
| self.delegate?.messageDismissed(message: self.currentMessage) | ||
| completionHandler?() | ||
| } | ||
| } |
There was a problem hiding this comment.
This is code that should only be called for modal messages. So I moved this block into the Modal manager subclass.
levibostian
commented
May 20, 2024
| @@ -65,5 +71,36 @@ public class InAppMessageView: UIView { | |||
| // } | |||
| } | |||
|
|
|||
| private func displayInAppMessage(_ message: Message) {} | |||
| private func displayInAppMessage(_ message: Message) { | |||
There was a problem hiding this comment.
This function's body is the logic that creates a WebView and renders the in-app message in the WebView. Most of this logic comes from InlineMessageManager. So all we need to do in the inline View is create a InlineMessageManager instance and setup the WebView for displaying in this View.
SDK binary size reports 📊SDK binary size of this PRSDK binary size diff report between this PR and the main branch |
Shahroz16
reviewed
May 21, 2024
levibostian
commented
May 21, 2024
levibostian
force-pushed
the
spr/main/60639329
branch
from
3c6243d to
2635ed8
Compare
levibostian
force-pushed
the
spr/main/025c7bf5
branch
2 times, most recently
from
ebabee3 to
4d63f69
Compare
Shahroz16
approved these changes
May 23, 2024
Ahmed-Ali
requested changes
May 23, 2024
Comment on lines
11
to
16
| class InlineMessageManager: MessageManager {} | ||
|
|
||
| /** | ||
| Class that implements the business logic for a modal message being displayed. | ||
| */ | ||
| class ModalMessageManager: MessageManager { |
There was a problem hiding this comment.
Why are we using inheritance here?
I understand there is common logic, but we can extract the common logic in its own utility type, and use it in two different implementations.
Inheritance obscures the side effect of doing a single change and can be hard to trace at times
There was a problem hiding this comment.
See my latest update.
| completionHandler?() | ||
| } | ||
| } | ||
| // expect subclass implements this. |
There was a problem hiding this comment.
I am honestly not big fan of subclassing when there are different approaches that makes the code easier to follow.
MessageManager can simply be a protocol with two different implementations if needed. The actual implementations can use similar utility functions for the common logic.
Inheritance is a very strong relation and unless absolutely a must, we can almost always find simpler approaches
There was a problem hiding this comment.
See my latest update.
levibostian
force-pushed
the
spr/main/60639329
branch
from
2635ed8 to
a7c9c1f
Compare
levibostian
force-pushed
the
spr/main/025c7bf5
branch
2 times, most recently
from
5458040 to
5581d89
Compare
|
I just pushed a change based on review suggestions. I kept I modified the There could be a better design pattern over this approach, but I think it will require more refactoring to the existing Modal in-app message code in the SDK. |
Part of: https://linear.app/customerio/issue/MBL-310/create-a-uikit-uiview-that-displays-an-inline-in-app-message-sent-from Create a WebView to display inline in-app message and display in the Inline UIView. To do this, we can re-use a lot of the same logic that Modal messages use to create WebViews and display them. Testing: There are some automated tests added in this commit. Reviewer notes: This commit will not show an in-app message if you were to add it to a sample app. Autolayout constraints need to be added to have Views resize and that will come in a future commit. commit-id:025c7bf5
levibostian
force-pushed
the
spr/main/025c7bf5
branch
from
5581d89 to
f0c5d97
Compare
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.
Part of: https://linear.app/customerio/issue/MBL-310/create-a-uikit-uiview-that-displays-an-inline-in-app-message-sent-from
Create a WebView to display inline in-app message and display in the Inline UIView. To do this, we can re-use a lot of the same logic that Modal messages use to create WebViews and display them.
Testing:
There are some automated tests added in this commit.
Reviewer notes:
This commit will not show an in-app message if you were to add it to a sample app. Autolayout constraints need to be added to have Views resize and that will come in a future commit.
Stack: