-
Notifications
You must be signed in to change notification settings - Fork 58
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
Setup Redux middleware for analytics tracking #4066
Conversation
Includes tracking for block insertion
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
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.
Great work! Thank you for putting this together. 🙌🏻
I left a few comments, but most of them are personal preference and not blockers. The only blockers would be innerBlocks
and that we need to make sure all the sibling branches ultimately align on things like the pattern of the string values sent to the host app. I would imagine we will wait to merge all of the PRs together after they are tested together.
I also tested this work (with a minor event name change from insertBlock
to editor_block_inserted
) against my current wordpress-mobile/WordPress-iOS#17243 and WordPress/gutenberg#35272 work — the event arrived in Tracks with the expected block_name
property. 🎉
Co-authored-by: David Calhoun <[email protected]>
We want these to be snake_case when they go to Tracks, and it seems easier to just make them snake_case here instead of having to convert them from camelCase to snake_case on the native side in both platforms.
( { name } ) => ( { block_name: name } ) | ||
); | ||
}, | ||
insertBlocks( blocks ) { |
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 can easily trigger insertBlock
, but I'm not sure how to trigger insertBlocks
from mobile. Any suggestions?
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.
From researching the usage of the insertBlocks
action creator, it would appear splitting at the end of some rich text blocks causes a insertBlocks
Redux action to trigger. E.g. placing a the cursor in a PullQuote block's citation input and pressing Return will trigger the action.
That said, neither WPAndroid or WPiOS implementations currently have sibling events defined for insertBlocks
, so the resulting analytics event will not be sent. I plan to add the required sibling events.
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.
Following up to share that my plan is to rename the editor_blocks_inserted
event to editor_block_inserted
rather than add sibling events to WPAndroid and WPiOS. This approach mimics what Calypso does, as both insertBlock
and insertBlocks
leverage the same event name.
To me, this approach makes more sense, as we our goal is tracking the insertion of a block. Whether the insertBlock
or insertBlocks
action creator is leveraged is implementation details. In fact, the insertBlock
action calls insertBlocks
itself.
…e into add/analytics-tracking
Because we recursively track individual blocks inserted, there is no value in tracking the implementation details of which action creator triggers the resulting block inserted analytic event. This mimics Calypso's approach and avoid leaking implementation details into the analytic events.
…e into add/analytics-tracking
…e into add/analytics-tracking
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.
🚀 LGTM. I verified functionality via wordpress-mobile/WordPress-Android#15406 (review).
Related PRs
Description
Fixes #4009. This adds the ability to send Redux actions over the RN bridge for processing on the host apps.
To test: See WPiOS and WPAndroid PRs.
PR submission checklist: