-
Notifications
You must be signed in to change notification settings - Fork 814
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
Upgrade Nudge: Fix redirect_to for Jetpack wp-admin #13165
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: August 6, 2019. |
f0712ff
to
9043ec6
Compare
ockham, Your synced wpcom patch D31062-code has been updated. |
ockham, Your synced wpcom patch D31062-code has been updated. |
ockham, Your synced wpcom patch D31062-code has been updated. |
? '/' + | ||
compact( [ postTypeEditorRoutePrefix, postType, getSiteFragment(), postId ] ).join( '/' ) | ||
: addQueryArgs( | ||
window.location.protocol + |
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.
Unfortunately, we don't seem to be exposing the site's protocol through any utilties available to JS (neither from Gutenberg/core nor Jetpack), so I'm falling back to window.location.protocol
.
Devil's advocate: Why even bother constructing the URL from 'reliable' data and not just window.location.href
for the entire redirect_to
URL I'm building?
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 seems to test well on Jetpack (I tested on both HTTP and HTTPS sites). I have not tested on WordPress.com though. I'll let someone else review that!
Required for #13081, and implementing musings found at #13081 (comment). Related: Automattic/wp-calypso#35027.
Changes proposed in this Pull Request:
Change the post-checkout redirect target to the wp-admin block editor for Jetpack sites, both in the editor (JS), and the frontend (PHP).
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Modifies the existing UpgradeNudge.
Testing instructions:
yarn build
), and start Docker (yarn docker:up
)docker/mu-plugins
, e.g. a newly created0-blocks-upgrade.php
(needs to start with<?php
):define ( 'JETPACK_SHOW_BLOCK_UPGRADE_NUDGE', true );
Verify that on WP.com, the redirect target is still the iframed Gutenberg editor (well, if you have enabled it, see PCYsg-hNB-p2):
Proposed changelog entry for your changes:
N/A (The UpgradeNudge hasn't been enabled in any previous JP version, so no need to add a ChangeLog entry.)