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 missing tracks events to product editor #38728
Conversation
Hi @octaedro, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
@@ -34,6 +34,7 @@ type MediaUploaderProps = { | |||
message: string; | |||
file: File; | |||
} ) => void; | |||
onMediaGalleryOpen?: () => void; |
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.
Added onMediaGalleryOpen
since onClick
is ambiguous in this case and limits how we redesign this component in the future.
@pmcpinto There's also click behavior on the background that opens a file uploader, but currently this only fires on the button click which opens the media gallery). Let me know if you'd like both.
@@ -23,6 +25,9 @@ export function PreviewButton( { | |||
const previewButtonProps = usePreview( { | |||
productStatus, | |||
...props, | |||
onClick() { | |||
recordEvent( 'product_preview_changes', { source: TRACKS_SOURCE } ); |
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'm wondering if we want this source
added to all events and if so, whether or not we should create a new recordProductEvent
(or enhance the current one we have) that wraps this and helps adding the source.
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.
Also, I'm wondering if the source
property should be _source
to help prevent potential conflicts.
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 like the idea of adding the source directly into the recordProductEvent
. Renaming source
to _source
makes a lot of sense as well.
Test Results SummaryCommit SHA: 979301b
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
@@ -87,6 +87,11 @@ export function Edit() { | |||
multipleSelect={ true } | |||
onError={ () => null } | |||
onFileUploadChange={ onFileUpload } | |||
onMediaGalleryOpen={ () => { | |||
recordEvent( | |||
'product_images_media_gallery_open' |
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 that the proposed name was wcadmin_product_add_image_button
, however wcadmin_product_images_media_gallery_open
is more specific.
Pinging @pmcpinto to let him know that we renamed the event.
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 job @joshuatf! The code looks good and is testing well here 🚀
The changes look good to me.
We don't need that one for now. Thanks for taking care of this so quickly. |
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.
} ) => { | ||
const [ isGuideOpen, setIsGuideOpen ] = useState( false ); | ||
const { isNewUser } = usePublishedProductsCount(); | ||
return ( | ||
<> | ||
<MenuItem | ||
onClick={ () => { | ||
recordEvent( | ||
'block_product_editor_about_the_editor_menu_item_clicked' |
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.
@pmcpinto Worth noting that this was overlapped with the product_dropdown_option_click
so I removed this event.
<ClassicEditorMenuItem | ||
productId={ id } | ||
onClick={ () => { | ||
recordClick( 'classic_editor' ); |
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 event also has a lot of overlap with product_editor_options_turn_off_editor_click
, but the latter has product_id
, product_type
, and product_status
.
Do we need both events, @pmcpinto?
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.
Thanks for the heads up. I didn't know this event existed. Having one is enough and I feel it's better to consolidate it under wcadmin_product_dropdown_option
. What do you think?
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.
Agreed! I'll update 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.
@pmcpinto Replaced product_editor_options_turn_off_editor_click
with wcadmin_product_dropdown_option
. Also added product_type
and product_status
to all of the dropdown option events.
I did not include product_id
in the event data as I'm not sure how useful it is, but please let me know if that's something that's used for joining data in tracks and I can include it here.
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.
@pmcpinto Replaced product_editor_options_turn_off_editor_click with wcadmin_product_dropdown_option. Also added product_type and product_status to all of the dropdown option events.
I did not include product_id in the event data as I'm not sure how useful it is, but please let me know if that's something that's used for joining data in tracks and I can include it here.
Thanks for the heads up. It's ok to skip product_id
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.
Good job @joshuatf! I just tested this PR after the rebase and the changes. LGTM
* Only record product update on already published products * Add source into published product tracks event * Add missing tracks options to product update event * Track tab clicks in the editor * Add source to tab clicks * Record tracks on preview click * Add events for media gallery, description, and attributes * Add tracks events to more menu * Replace new_product_page with source * Add changelog entry * Fix lint errors * Add tracks for About the Editor menu item * Add onClick event to AboutTheEditor menu item * Use dropdown option event for classic editor menu click
* Add missing tracks events to product editor (#38728) * Only record product update on already published products * Add source into published product tracks event * Add missing tracks options to product update event * Track tab clicks in the editor * Add source to tab clicks * Record tracks on preview click * Add events for media gallery, description, and attributes * Add tracks events to more menu * Replace new_product_page with source * Add changelog entry * Fix lint errors * Add tracks for About the Editor menu item * Add onClick event to AboutTheEditor menu item * Use dropdown option event for classic editor menu click * Prep for cherry pick 38728 --------- Co-authored-by: Joshua T Flowers <joshuatf@gmail.com> Co-authored-by: WooCommerce Bot <no-reply@woocommerce.com>
Submission Review Guidelines:
Changes proposed in this Pull Request:
Adds tracks discussed in the issue as well as missing properties to fill in missing gaps in tracks data. Also creates a
source
property to better identify where events are triggered from.@pmcpinto made the following changes to the events suggested:
product_add_description_button
->product_add_description_click
product_add_attribute_button
->product_add_attributes_click
(note I made the assumption that this event is fired when the "Add attributes" button is clicked to open the attributes modal)product_dropdown_option
->product_dropdown_option_click
(options:feedback
|classic_editor
)Closes #38637 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
localStorage.setItem( 'debug', 'wc-admin:tracks' );
wcadmin_product_add_publish
will fire on the backend and can be found under WooCommerce -> Status -> Logs and finding the latesttracks-2023-xx-xx
logs.