-
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
Show the correct homepage when previewing a theme in the Site Editor #31929
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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:
Mu Wpcom plugin:
|
f94567b
to
70d6ff5
Compare
bd03f06
to
68b7640
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.
Testes:
Atomic sites: ✅ BTP shows Index of Home template as homepage.
Simple sites: ✅ BTP works as before.
Self-hosted sites: ✅ BTP works as before.
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'm not sure I fully understand why this is in Jetpack. I think I'm missing some context on this, probably.
If we always want to show the posts page when previewing a new theme, shouldn't we make that change in Core directly, since theme previews will soon be available to everyone?
@jeherve Based on pekYwv-1BS-p2 and pekYwv-1BS-p2#comment-1187, I guess Core won't touch the reading settings in the short term since Core won't change the reading settings during the theme switch. So, it makes sense to keep the current reading settings when you're previewing a theme in the site editor on Core. However, dotcom has a mechanism to update the reading settings based on the headstart annotation of the theme. So, we have to override the Does it make sense? Or any thoughts? |
Ok, that makes sense. In this situation, and since this is really only interesting for WordPress.com sites (simple and WoA), maybe this should be shipped in the mu-wpcom plugin instead? |
projects/packages/jetpack-mu-wpcom/src/features/block-theme-previews/block-theme-previews.php
Show resolved
Hide resolved
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.
👍
24a67c3
to
9ad0326
Compare
9ad0326
to
c4864d0
Compare
@Automattic/lego I requested your reviews again as I resolved conflicts 🙏 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.
Tested by using the following scenarios:
- current theme: Appleton, which has a static homepage set.
- previewing theme: Pendant, which has the Home template.
Atomic site ✅
Before | After |
---|---|
Simple site ❌
Before | After |
---|---|
I am not sure what I'm doing wrong. I have sandboxed my site (see the "Sandboxed" badge at the left-corner of the right screenshot). Log:
$ bin/jetpack-downloader test jetpack-mu-wpcom-plugin add/site-editor-homepage
Current checkout is switch-theme-logic-new. Check out trunk? [Y/n] Y
Updating trunk...
# Request received by "phabricator-b06-29.dfw.automattic.com", forwarding to cluster host "phabricator-b12-10.dfw.automattic.com".
# Acquiring read lock for repository "wpcom-git" on device "phabricator-b12-10.dfw.automattic.com"...
# Acquired read lock immediately.
# Device "phabricator-b12-10.dfw.automattic.com" is already a cluster leader and does not need to be synchronized.
# Cleared to fetch on cluster host "phabricator-b12-10.dfw.automattic.com".
Updating files: 53% (12198/23014)
Updating files: 61% (14039/23014)
Updating files: 100% (23014/23014), done.
Determining jetpack-mu-wpcom-plugin download URL...
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 10406 100 10406 0 0 47733 0 --:--:-- --:--:-- --:--:-- 47733
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 8181 100 8181 0 0 71763 0 --:--:-- --:--:-- --:--:-- 72398
Downloading jetpack-mu-wpcom-plugin...
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 80835 100 80835 0 0 400k 0 --:--:-- --:--:-- --:--:-- 398k
Unpacking jetpack-mu-wpcom-plugin...
jetpack-mu-wpcom-plugin add/site-editor-homepage is now checked out in sun!
If you intend to commit it, use `git add -Af wp-content/mu-plugins/jetpack-mu-wpcom-plugin/sun` to ensure all files are added.
To reset your environment instead, use `jetpack-downloader reset jetpack-mu-wpcom-plugin`.
Will test on a JN site soon.
This is weird. 👀 I will check it Monday! |
Jetpack-connected self-hosted site ✅Nothing breaks! |
Gave a test on Simple Sites, and it works as expected 👍 |
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.
Proposed changes:
Show the correct homepage when previewing a theme in the Site Editor to support Block Theme Previews on dotcom. This is done by overriding the
show_on_front
option toposts
.I added changes in
Jetpackjetpack-mu-wpcom since dotcom wants to support Block Theme Previews both on Simple and Atomic sites.This PR is part of Automattic/wp-calypso#79221.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Atomic sites
define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true );
to yourwp-config.php
/wp-admin/site-editor.php?wp_theme_preview=<your-theme>
(e.g.?wp_theme_preview=pendant
)Simple sites
public-api.wordpress.com
/wp-admin/site-editor.php?wp_theme_preview=<your-theme>
(e.g.?wp_theme_preview=pub/pendant
)self-hosted site
jetpack rsync
PCYsg-eg0-p2/wp-admin/site-editor.php?wp_theme_preview=<your-installed-theme>
(e.g.?wp_theme_preview=twentytwentytwo
)