chore: when in-app web content rendered, animate visibility of inline View #724
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
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
changed the title
SDK binary size reports 📊SDK binary size of this PRSDK binary size diff report between this PR and the main branch |
levibostian
force-pushed
the
levi/inline-animate-open
branch
2 times, most recently
from
3666239 to
8475968
Compare
Ahmed-Ali
approved these changes
May 28, 2024
There was a problem hiding this comment.
Thank you Levi for all the work and the impressive outcome.
Non-blockers:
- In the sample app, it might be worth it to create separate sub-screen where you can put variety of examples that don't have to match the dashboard layout. This would be great for testing different use cases and also testing.
Blockers:
We need to test that model views are still working. A test video showing the scenarios used to verify inline and model views working would also a neat way to show the changes.
I am approving the PR so once you address the above comments, you don't have to wait long before merging.
Comment on lines
+11
to
+17
| while true { | ||
| if let nextLevelSuperview = rootSuperview.superview { | ||
| rootSuperview = nextLevelSuperview | ||
| } else { | ||
| return rootSuperview | ||
| } | ||
| } |
There was a problem hiding this comment.
Can be simplified a bit
Suggested change
| while true { | |
| if let nextLevelSuperview = rootSuperview.superview { | |
| rootSuperview = nextLevelSuperview | |
| } else { | |
| return rootSuperview | |
| } | |
| } | |
| while rootSuperview.superview != nil { | |
| rootSuperview = rootSuperview.superview | |
| } | |
| return rootSuperview |
levibostian
force-pushed
the
levi/display-inline-uiview-afterfetch
branch
2 times, most recently
from
3dfbe5a to
69ef079
Compare
levibostian
force-pushed
the
levi/inline-animate-open
branch
from
8475968 to
fbc5274
Compare
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
…UIKit app Part of: https://linear.app/customerio/issue/MBL-310/create-a-uikit-uiview-that-displays-an-inline-in-app-message-sent-from This commit implements UIKit AutoLayout constraints to the Inline View and the WebView. Adding these constraints make inline messages visible inside of UIKit apps! The Inline View will start at height 0 and then after the webview content is loaded, it will instantly change the height to display the full in-app message. Testing: All testing is done manually in sample apps. AutoLayout constraints change the visuals of Views so viewing how the View looks in an app is required to test. If you want to test this commit in a sample app build, follow these instructions: * Open the UIKit app on device. * Immediately after the app opens, send yourself a test in-app message with element ID "test". * Wait 10 seconds. In this time, the in-app SDK will fetch the test in-app message and then will display it inline on the screen. commit-id:5ab2c403
Part of: https://linear.app/customerio/issue/MBL-310/create-a-uikit-uiview-that-displays-an-inline-in-app-message-sent-from Fetching of in-app message is an async operation running in the background. There is a chance that in-app messages are not available when the inline View is constructed but become available after. This commit gets notified when a fetch is complete so it can check if there are in-app messages to display after the View has already been constructed. Testing: * Automated tests are added. * Verified that messages display in sample app. If you want to test this commit in a sample app build, follow these instructions: 1. Kill the app on device. 2. Send yourself a test inline message from Fly. 3. Open app. After you wait a few seconds, the inline message you sent yourself will display. commit-id:aea90dff
levibostian
force-pushed
the
levi/display-inline-uiview-afterfetch
branch
from
69ef079 to
f96607f
Compare
… View Part of: https://linear.app/customerio/issue/MBL-311/provide-positive-ux-when-inline-messages-displayed-by-animating-height Fetching of in-app message is an async operation running in the background. There is a chance that an inline in-app message is rendered and ready to display on the screen currently in the foreground. We want to display the inline message when it's ready to show, and we also want to display these messages without causing a negative UX. This commit does the following: 1. Hides the inline View when the View is constructed by setting the height to 0. 2. When the in-app message web content is rendered, the inline View is given a height that matches the web content aspect ratio. The inline View animates it's height from 0 to the new height to make the inline View visible. Testing: This change is heavy on UI so testing is done mostly in UIKit sample app QA testing. If you want to test this commit in a sample app build, follow these instructions: 1. Open the app on device. 2. Send yourself a test inline message from Fly to one of the inline Views on Dashboard screen. 3. After you wait a few seconds, you will see the inline message animate into visibility. Notes for reviewer: * During development, I encountered difficulties with getting the animation to work when the inline View is embedded in a StackView. Because of this, I modified the Dashboard screen's UI to nest the inline Views inside of the StackView to make sure we QA test this scenario.
levibostian
force-pushed
the
levi/inline-animate-open
branch
from
fbc5274 to
434d587
Compare
The logic is actually backwards. I modified the test with the expected behavior.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature-inline-inapp #724 +/- ##
=======================================================
Coverage ? 58.83%
=======================================================
Files ? 143
Lines ? 4016
Branches ? 0
=======================================================
Hits ? 2363
Misses ? 1653
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Base automatically changed from
levi/display-inline-uiview-afterfetch
to
feature-inline-inapp
June 11, 2024 19:09
|
0 failed tests on CI, even though the run failed. Going to merge this. |
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-311/provide-positive-ux-when-inline-messages-displayed-by-animating-height
Fetching of in-app message is an async operation running in the background. There is a chance that an inline in-app message is rendered and ready to display on the screen currently in the foreground. We want to display the inline message when it's ready to show, and we also want to display these messages without causing a negative UX.
This commit does the following:
Testing:
This change is heavy on UI so testing is done mostly in UIKit sample app QA testing. If you want to test this commit in a sample app build, follow these instructions:
Notes for reviewer:
Demo:
Untitled.mov
Sorry, the tool I use for making stacked PRs,
spr, was not able to create this PR for me. To help navigate the PR stack, here are some of the previous PRs in the stack:Stack: