-
Notifications
You must be signed in to change notification settings - Fork 134
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
REPLAY-1516 Unsupported ViewController placeholder #1243
REPLAY-1516 Unsupported ViewController placeholder #1243
Conversation
Add snapshot test |
c131e4a
to
7f48846
Compare
4b8e08e
to
31b781f
Compare
As discussed - removed snapshot tests for these |
31b781f
to
c02076c
Compare
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 get the idea 👌, but backward iteration over responder chain for each view is too slow (it adds +20% to main thread time in some measured cases). We need a different approach IMO - I left one suggestion.
...wTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UnsupportedViewControllerRecorder.swift
Outdated
Show resolved
Hide resolved
c02076c
to
f7393fa
Compare
Depends on this: |
296238f
to
949ad27
Compare
949ad27
to
c5b1e04
Compare
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.
💪 It looks good in overall 👌. I left few suggestions, with spotting two places in tests that will become flaky ❄️ once we start comparing snapshot images on CI (coming pretty soon).
DatadogSessionReplay/SRSnapshotTests/SRHost/Fixtures/BasicViewControllers.swift
Outdated
Show resolved
Hide resolved
DatadogSessionReplay/SRSnapshotTests/SRHost/Fixtures/BasicViewControllers.swift
Outdated
Show resolved
Hide resolved
...corder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UnsupportedViewRecorder.swift
Show resolved
Hide resolved
...corder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UnsupportedViewRecorder.swift
Outdated
Show resolved
Hide resolved
...ay/Sources/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/ViewTreeRecordingContext.swift
Outdated
Show resolved
Hide resolved
...lay/Sources/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/ViewTreeSnapshotBuilder.swift
Outdated
Show resolved
Hide resolved
Thanks for great review! I was exploring static content, but unfortunately it's only possible for web view (but not safari vc). Made some changes. Pushing soon :) |
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.
👏 🤜 🤛
private protocol AnyUIHostingViewController: AnyObject {} | ||
@available(iOS 13.0, *) | ||
extension UIHostingController: AnyUIHostingViewController {} |
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.
Nice 🥇
What and why?
Adds placeholders for certain view controllers.
How?
It's done by figuring out the private view class for each unsupported VC.
In that case we ignore it and put a placeholder wireframe in place.
Review checklist
Custom CI job configuration (optional)