-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Track Load WordPress screen events and content export events #19719
Conversation
UIApplication.shared.open(schemeUrl) | ||
} | ||
actions.secondary = { [weak self] in | ||
loadWordPressViewController.dismiss(animated: true) { | ||
actions.secondary = { [weak self, weak loadWordPressViewController] 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.
Adding [weak loadWordPressViewController]
to break a reference cycle between the secondary action handler and loadWordPressViewController
.
Without the weak
reference, the memory would look like this:
loadWordPressViewController
-> loadWordPressViewModel
-> actions.secondary
-> loadWordPressViewController
a -> b
means a
owns b
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! I'd had a hunch something in this area would cause a reference cycle but didn't go back to investigate.
You can test the changes in WordPress from this Pull Request by:
|
You can test the changes in Jetpack from this Pull Request by:
|
Generated by 🚫 dangerJS |
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
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!
case .ineligible: return "Content export is ineligible" | ||
case .exportFailure: return "Content export failed" | ||
case .localDraftsNotSynced: return "Local drafts not synced" | ||
} |
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.
Not that it matters but I think general style if it exists is to put them on new lines 😀
Thanks @alpavanoglu and @dvdchr for your review 🙇 |
Part of #19701 and #19640
Description
This PR adds tracking of the following migration events:
Test Instructions
Load WordPress Screen Events
Display Load WordPress Screen
Case 1: Tapping Open WordPress Button
Case 2: Tapping No Thanks Button
Content Export Events
Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
None.
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.