-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add Tracks Analytics on frontend #185
Conversation
Co-authored-by: Chris Zarate <chris.zarate@automattic.com>
src/blocks/remote-data-container/components/field-shortcode/FieldShortcodeButton.tsx
Outdated
Show resolved
Hide resolved
src/blocks/remote-data-container/components/modals/InputModal.tsx
Outdated
Show resolved
Hide resolved
@@ -66,6 +66,7 @@ function BoundBlockEdit( props: BoundBlockEditProps ) { | |||
attributes={ attributes } | |||
availableBindings={ availableBindings } | |||
blockName={ blockName } | |||
remoteDataName={ remoteDataName } |
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.
Instead of prop drilling the container block name, I think we should just prop drill the sendTracksEvent
function with the data_source_type baked in
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'll need prop drilling in either case, to me passing blockName
seems simpler than passing the function.
These changes improve the overall structure and maintainability of the codebase.
… REMOTE_DATA_BLOCKS const
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 like a really good start. Some minor pedantic things I might do a little different, but not worth nit picking. I like it.
@@ -51,11 +55,23 @@ export function BlockBindingControls( props: BlockBindingControlsProps ) { | |||
function updateFieldBinding( target: string, field: string ): void { | |||
if ( ! field ) { | |||
removeBinding( target ); | |||
sendTracksEvent( 'remotedatablocks_remote_data_container_actions', { | |||
action: 'remove_binding', | |||
data_source_type: getBlockDataSourceType( remoteDataName ), |
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.
Should this also have a remote_data_field
property?
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.
For remove_binding
action we don't get field
value and that's why I haven't included remote_data_field
in track props. Let me know if you think we should include it, I will find an alternative.
Pull request #185 has caused an issue with the withBlockBinding tests, which attempt to use the Tracks API. To resolve this, the Tracks function was mocked.
Description
Add Tracks Analytics by using the
@automattic/calypso-analytics
package. With each track event, the following properties will be tracked:Note: This tracking will only work if site is hosted on VIP Platform or if the analytics is enabled via filter.
How to test?
Create a site using
vip dev-env
which will have the MU Plugins.Move
remote-data-blocks
plugin to theplugins
folder.Add filter in
plugin-loader.php
to enable tracking.Checkout this wp-calypso PR,
build
andlink
the@automattic/calypso-analytics
package.To allow tracking on local env comment check on PHP and JS side.
Trigger track events, use "Live View" to verify that the events are getting logged.