-
Notifications
You must be signed in to change notification settings - Fork 37
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
chore: support to session replay #123
Conversation
sessionReplay = it | ||
} | ||
|
||
this.sessionReplayConfig.captureLogcat = false |
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.
flutter has its own logging system, so no need to watch logcat
val id = call.argument<Int>("id") ?: 1 | ||
val x = call.argument<Int>("x") ?: 0 | ||
val y = call.argument<Int>("y") ?: 0 |
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.
its ok to have a fallback here but it should never be the case anyway, x and y are always 0, and the id is ok to be 1
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.
what is really important is imageBytes
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.
yes make sense
@@ -1,2 +1,3 @@ | |||
org.gradle.jvmargs=-Xmx4G | |||
android.useAndroidX=true | |||
android.suppressUnsupportedCompileSdk=34 |
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.
needed for latest versions and we want to keep back compatibility so not bumping the sample
@@ -1,7 +1,7 @@ | |||
import UIKit | |||
import Flutter | |||
|
|||
@UIApplicationMain | |||
@main |
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.
this is new but its the sample so its ok
properties: properties, | ||
); | ||
}) { | ||
_currentScreen = screenName; |
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.
regression introduced here during refactoring https://github.com/PostHog/posthog-flutter/pull/117/files#diff-76c6e9a0e4d6150266a5008a193f88cea761a5620afecce9ba9ed7839aeb32c1
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.
Looks good to me
@pauldambra @ioannisj ready for review =) |
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'll probably need to test this locally as well - but left a few minor comments here and there. Awesome work!
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.
Awesome! :D
💡 Motivation and Context
Closes #69
Continues #116
Depends on PostHog/posthog-ios#266
💚 How did you test it?
Running sample on Android
📝 Checklist