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 tracking when opening or searching in the Command Palette #41838
Conversation
Hi @dinhtungdu, 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: |
1 similar comment
Hi @dinhtungdu, 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: |
e032348
to
b878d43
Compare
Test Results SummaryCommit SHA: 88e468a
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. |
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.
Code looks and tests good in my test so I'm pre-approving! Thank you for working on this.
I have a question on the palette open event. Do we record where (which screen/page) the palette is opened? From the debug value I didn't see any data related to the current screen. I also see a property named shouldRecord
set to false
, which is a bit confusion and makes me wonder if the event was recorded or not.
Thanks for the review, @dinhtungdu!
Good idea! I added an Btw, I added this property to all events, not only the open one. That means also when submitting one command or when searching. I think it might be useful in those cases as well, but cc'ing @pmcpinto in case he has other thoughts.
I think that's expected if you are in development mode or don't have tracking enabled: woocommerce/packages/js/tracks/src/index.ts Lines 38 to 42 in be79b98
|
b878d43
to
4ed1d57
Compare
4ed1d57
to
a0c8922
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 update looks good to me! Tested and I can confirm it's working, the origin
data is tracked correctly.
a0c8922
to
e445d26
Compare
e445d26
to
88e468a
Compare
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #41837.
This PR adds tracking when the user opens the Command Palette and makes a search in it.
cc @pmcpinto
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:*' );
.wc-admin:tracks recordevent wcadmin_woocommerce_command_palette_open
).products
.wc-admin:tracks recordevent wcadmin_woocommerce_command_palette_search { value: "products", origin: "post-editor" }
).Changelog entry
Significance
Type
Message
Comment
Add tracking when opening or searching in the Command Palette