This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
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.
Analytics opt in for posthog #6936
Analytics opt in for posthog #6936
Changes from 82 commits
fdd8215
b136599
a90e49e
0178bcc
1942963
e9f0741
7def9c2
ff0726d
3547e91
c3b200b
d9ce993
6bd3c57
d9baaca
79c26bd
9e0f4a7
fa19b8a
a9fe002
b85a44e
76978a4
cd287fa
4216807
5612223
3cea363
4e0a973
5caf52b
3021333
f42d8b2
fbc38b3
17b86d6
5464742
38044a8
ec76c88
f620f89
d2ea642
7e1630f
c6a627b
2f5ab38
fd8d79c
b93c2a1
eba44d9
571f986
67fd6c5
ec6d829
26b537e
f35dfb4
40b1179
0070fd1
9667d0e
f155482
0a1cb02
c950db5
af11ff7
e75ccaa
becb105
dbeaadd
7c68aaa
3cb2c23
14f5523
3c40684
d9fedf6
a550b66
c256f72
72ad384
432c980
ec9c6ba
18a0654
269e786
98d156f
dfd360c
806545b
ed1c2b5
73db237
6628dbf
196d0d2
01c9574
0adedfc
560494f
dab34a7
1264c57
9eb010d
5bfafa7
1017e71
2a34286
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
we have $font-semi-bold for this
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.
"Some examples" feels like we're hiding something when we're not - the list should be exhaustive. Not a blocker for merge, but this did catch me off guard.
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 intend to collate a full data dictionary which will denote example what each event and property does - primarily so people trying to do analysis understand what events correspond to, but a secondary benefit would be that it'll be very transparent what we're tracking.
Note that the previous wording was "The information being sent to us to help make %(brand)s better includes", and what is there now is quite partial (we track 29 events through Matomo, and they and their properties not documented here - this I think is just properties sent with every event).
So I actually think drawing a bit more attention to the weaselness is clarifying.
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.
@novocaine once you've completed that list are you able to share with me so that it can be replicated on the cookie policy?
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.
We are recording the data dictionary over here in JSON schema. Clients cant send events without adding to this schema.
So, we can generate something human readable from this. I'll update you once that's complee.
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.
readonly feels awkward for this sort of field (does it really never change from 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.
I think this reinforces the semantics nicely - it only ever needs to gets decided once in the constructor (allowed by
readonly
semantics) because its set based on something that can't change at runtime (sdk config)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.
can we get a comment to say that it's decided in the constructor? Our typical pattern isn't to create new instances as needed, but rather toggle an internal flag.
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 does have an existing comment, happy to reword it if you have a suggestion that'd make it clearer