-
Notifications
You must be signed in to change notification settings - Fork 798
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
Site Editor: dashboard link compat for Gutenberg 14.5 #27601
Conversation
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
TestsI commented out the dashboard link logic inside of ETK
I disabled ETK plugin and switched to this Jetpack PR using Jetpack Beta tester.
|
Once the build is complete, you run There's an automated comment that reminds you, in case you forget. |
@anomiex thank you! |
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 like this will only affect wpcom Simple and Atomic, and I see someone has already tested those cases.
Left an inline comment, but it probably should not actually be applied in this PR for the reason given therein.
* @return array Updated Editor settings. | ||
*/ | ||
public function site_editor_dashboard_link( $settings ) { | ||
$settings['__experimentalDashboardLink'] = 'https://wordpress.com/home/' . $this->domain; |
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.
Normally I'd suggest doing something like this here, just in case the value contains something weird.
$settings['__experimentalDashboardLink'] = 'https://wordpress.com/home/' . $this->domain; | |
$settings['__experimentalDashboardLink'] = 'https://wordpress.com/home/' . rawurlencode( $this->domain ); |
But it looks like there's a ton of other code in this file already not doing that, and the value is even being munged to replace /
characters (probably for exactly that lack), so doing it only here would be inconsistent and might even break something depending on the unencoded behavior.
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.
Yeah keeping consistent is usually better than being "right", but good to notice 👍
Gutenberg 14.5 has removed the slotfill we formerly used for the dashboard link override, instead introducing a __experimentalDashboardLink setting. This replaces Automattic/wp-calypso#69929
9c419c6
to
433740a
Compare
@anomiex the "PR is up to date" job was stuck in a weird state (it said here that it failed but clicking the Details link led to results from another PR) so I just did a rebase and pushed the branch back up and of course that dismisses your approving review! And now after that, the Anyway, need another quick 👍 or you can just merge this yourself. |
The "PR is up-to-date" check runs in two different contexts. When the PR itself is updated it runs to check the PR, and when the tags it checks against are updated it runs to check every open PR. Probably the weird state was the latter case. I think the |
BTW, a rebase will always dismiss existing reviews, while if you do a trunk merge instead it often keeps them. |
Thanks again @anomiex ! |
* @return array Updated Editor settings. | ||
*/ | ||
public function site_editor_dashboard_link( $settings ) { | ||
$settings['__experimentalDashboardLink'] = 'https://wordpress.com/home/' . $this->domain; |
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.
It looks like this may be causing this issue Automattic/wp-calypso#71760 .
In short in testing flows on horizon, leaving the site editor on horizon now bumps you into the production environment.
Introduced in #69929 Replaced by Automattic/jetpack#27601
Introduced in #69929 Replaced by Automattic/jetpack#27601
Gutenberg 14.5 has removed the slotfill we formerly used for the dashboard link override, instead introducing a __experimentalDashboardLink setting.
This replaces Automattic/wp-calypso#69929
Fixes Automattic/wp-calypso#70074
Changes proposed in this Pull Request:
< Dashboard
link to WP.com HomeOther information:
Jetpack product discussion
n/a
Does this pull request change what data or activity we track or use?
no
Testing instructions:
WPCom Simple
run bin/jetpack-downloader test add/site-editor-dashboard-compat
< Dashboard
link points towordpress.com/home/SITE_SLUG
rather thanSITE_SLUG.wordpress.com/wp-admin/site-editor.php
WoA
NOTE: given that this supersedes Automattic/wp-calypso#70074 it will not necessarily appear to change anything on WP.com Simple, since both filters will be simultaneously operable. We get a truer test on Atomic by disabling the ETK plugin.