-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Improve async checklist code. Follow-up to #27926 #28678
Conversation
this.props.track( 'calypso_checklist_banner_close', { | ||
site_id: siteId, | ||
} ); | ||
} |
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 original author clearly had intent to send the calypso_checklist_banner_close
tracks event, but then something went wrong 😄 Let's make the tracks
call work great again.
It got broken in this commit: 29af3b0. Part of #26764 by @ockham and @sirreal.
That commit removes file my-sites/stats/checklist-banner/index.jsx which connected the track
prop to recordTracksEvent
action.
And creates a new file, my-sites/checklist/wpcom-checklist/checklist-banner/index.js, which still calls this.props.tracks
, but doesn't do the connect. ⚡️💥
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 spotting this 🙇
Will note the affected period: 623-gh-tracks-events-validation-status
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 the latest commit adds it back in correctly! :) Could one of you please double-check the code looks good?
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 good 👍
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, @jsnajdr ! Also, by the way, excellent sleuthing work!
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.
Ready to 🚢 if the tracks
prop gets fixed The Right Way™ Thanks for making the checklist code better!
Changes proposed in this Pull Request
Code improvements to async checklist merged in #27926 as per the review in that PR. It was decided these could be done after merging, to reduce the time the PR stayed open and generating merge conflicts.
Testing instructions
Try the various checklist variations and ensure that the behaviour remains the same.