-
Notifications
You must be signed in to change notification settings - Fork 847
Social Previews: Add upgrade path #16797
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
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.
Could we prioritize reviewing this PR before these changes? This PR adds and getUpgradeUrl() helper among other changes. If not, it seems we are going to overlap efforts.
Thinking well not sure about :this: I guess getting the |
18a129a to
8ebc9b2
Compare
946ab9a to
d185e84
Compare
marekhrabe
left a comment
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 have fixed one tiny detail - the tracking call was done directly in the render function and that means it was easily called 5 times instead of once. I have wrapped it in an effect and now it's only called once per component lifecycle.
This works well for me now. Can you provide more detail @retrofox about the concerns you mentioned? I'm not entirely sure where you are heading or whether they are still relevant. Let me know so we can either proceed with this PR or plan a change.
551b04a to
0f70853
Compare
9aaa4fe to
a8d6905
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.
This is coming together! 👍
Did a quick code review. Noticed some minor things.
Also, this PR is quite difficult to review because it contains lots of changes that seem unrelated to this PR 🤷
We should probably rebase this now right?
9037f1c to
f431f4c
Compare
|
Caution: This PR has changes that must be merged to WordPress.com |
This is an automated check which relies on E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16797 |
|
@getdave Thanks for you review! I rebased the PR and addressed most of your feedback. I think this is ready for another round. |
Takes inspiration from `UpgradeNudge` and `BlockNudge` to save the post and redirect to the checkout flow of the Business plan.
Props @getdave.
…icks on button This was added in the premium blocks branch `feature/premium-blocks` and brings it inline with that version. You can use the new `isRedirecting` boolean to disable the button to provide visual feedback.
Also simplifies button text and updates the way required_plan is extracted.
aeb084b to
bf679c7
Compare
jeherve
left a comment
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.
Looking good. 🚢
|
r212283-wpcom |
* [not verified] Social Previews: Add upgrade path Takes inspiration from `UpgradeNudge` and `BlockNudge` to save the post and redirect to the checkout flow of the Business plan. * [not verified] fix repeated call of tracking * Address PR feedback Props @getdave. * use-upgrade-flow: first approach * plan-utils: add getUpgradeUrl() helper fn * social-preview: refact -> use useUpgradeFlow hook * Use Jetpack_Editor_Initial_State as source of truth * use-upgrade-flow: Add "isRedirecting" feature, to prevent multiple clicks on button This was added in the premium blocks branch `feature/premium-blocks` and brings it inline with that version. You can use the new `isRedirecting` boolean to disable the button to provide visual feedback. * social-preview upgrade: Give visual feedback when redirecting * use-upgrade-flow: set async callback fn * social-preview: do not track nudge event * Use required_plan for click tracking Also simplifies button text and updates the way required_plan is extracted. * Avoid snake case * Run prettier * getUpgradeUrl: minor documentation fix (no functional changes) Co-authored-by: Marek Hrabe <[email protected]> Co-authored-by: retrofox <[email protected]> Co-authored-by: Matthew Reishus <[email protected]>
Takes inspiration from
UpgradeNudgeandBlockNudgeto save the post and redirect to the checkout flow of the Business plan.Reuses the following event:
jetpack_editor_block_upgrade_click. We'll do a follow up PR for view event tracking.Fixes #16781.
Changes proposed in this Pull Request:
Does this pull request change what data or activity we track or use?
Similar to
UpgradeNudgetracking.Testing instructions:
follow setup steps from Social Preview: Add scaffolding for the feature #16615.
When testing on dotcom, make sure D48017-code is applied.No longer required as now in Production.this PR currently shows both paid and unpaid versions of the sidebar. Click the one withIf tested on local Jetpack Plugin, you will need to comment outsocial-previewshere in order to activate the "upgrade nudge".Click the "Learn More" button and it opens up this upgrade nudge
Click on "Upgrade" to see the post being saved before being redirected to the checkout flow.
Additionally, confirm that both events are recorded when you do this flow:
Set the debug with
localStorage.setItem( 'debug', 'dops:analytics*' );ProTip: Preserv the log in order to see the second event when the redirect happens.
Proposed changelog entry for your changes: