-
Notifications
You must be signed in to change notification settings - Fork 2
feat: implement custom dimension tracking #31
Conversation
c3e3322
to
d1867e1
Compare
d1867e1
to
7799060
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.
A good start. Would be nice to minimise the platform differences though, or at least write more documentation about them.
android/src/main/java/com/reactnativepiwikprosdk/PiwikProSdkModule.kt
Outdated
Show resolved
Hide resolved
b83a226
to
27b58a4
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.
The scope term still seems a bit ambiguous. I personally think it would be simplest to just limit setting the dimension to single track*
functions, similarly to as proposed in #18 and what the Android SDK does https://developers.piwik.pro/en/latest/sdk/Piwik_PRO_SDK_for_Android.html#tracking-custom-dimensions. If there is a need for sticky custom dimensions they can maybe be implemented separately, or just handled by the library caller.
android/src/main/java/com/reactnativepiwikprosdk/PiwikProSdkModule.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativepiwikprosdk/PiwikProSdkModule.kt
Outdated
Show resolved
Hide resolved
9340a33
to
df0ab9c
Compare
df0ab9c
to
7c7c43a
Compare
60f95b4
to
e9f75c9
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.
Implementation looking good now! Thanks for your efforts. Added some mentions about small things that could still be fixed if you feel up to it. Some more documentation, with maybe an example in README.md
, might be nice to have as well.
Nonetheless, ready to merge this as it is if you don't happen to have more time to contribute.
Implements: #18