-
Notifications
You must be signed in to change notification settings - Fork 21
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 (budget and audiences) tracking for Onboarding completed with Ads #2167
Conversation
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 work.
There are questions about the call timing of event tracking and whether to merge event tracking. I think they need to be clarified first.
Just noticed another thing. Event tracking also involves extension users. Should it belong to the |
…_paid_ads_button_click
Howdy @eason9487 Thanks for reviewing this. I applied your suggestion as it fixes the race problem and reuses the I updated also from Can you help with a new round of reviews? |
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 adjustment.
It still needs a bit of tweak as the initial paidAds.countryCodes
is undefined
.
Hi @eason9487 thanks again for the review and for the catch. Sorry. I saw that error yesterday and forgot to push it 😵💫 Can you do a new round? Thanks. |
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
Changes proposed in this Pull Request:
This is a continuation for #2153
This PR adds the Tracking events with the budget and the audience when the onboarding is completed adding Ads.
Screenshots:
Detailed test instructions:
localStorage.setItem( 'debug', 'wc-admin:*' )
Additional details:
Changelog entry