-
Notifications
You must be signed in to change notification settings - Fork 796
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
Calypsoify: Requiring the Calypsoify package in the Jetpack and Mu-wpcom plugins #37375
Conversation
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 appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
…, to utilize Jetpack and helper methods
'../dist/index.js', | ||
__FILE__, | ||
array( | ||
'jquery', |
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 think you meant like this here
'jquery', | |
'dependencies' => array( 'jquery' ), |
OTOH, if you were to add an import jQuery from 'jquery';
to the JS file, then it would show up in the index.asset.php
file and you could omit specifying it here.
But if you're going to update the script, even better would probably be to replace the $( 'some selector' )
with document.querySelector()
or document.querySelectorAll()
, and then the .attr()
calls with direct accesses to .href
and .target
, so we can avoid requiring jquery at all. 🙂 And probably also replace the uses of wp.data
with import { dispatch, select } from '@wordpress/data';
.
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.
Good idea :)
@@ -97,6 +99,11 @@ public static function load_features() { | |||
if ( class_exists( 'Automattic\Jetpack\Scheduled_Updates' ) ) { | |||
Scheduled_Updates::init(); | |||
} | |||
|
|||
// Gets autoloaded from the Scheduled_Updates package. | |||
if ( class_exists( 'Automattic\Jetpack\Calypsoify\Jetpack_Calypsoify' ) ) { |
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 works the same but IMO is more readable.
if ( class_exists( 'Automattic\Jetpack\Calypsoify\Jetpack_Calypsoify' ) ) { | |
if ( class_exists( Jetpack_Calypsoify::class ) ) { |
Although... when wouldn't the class exist, since the package is being required in composer.json?
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.
Good point, I just followed the previous process, but then I see the one above does not check for the class (\Marketplace_Products_Updater::init();
). Will change that.
$( 'body.revision-php a' ).each( function () { | ||
var href = $( this ).getAttribute( 'href' ); | ||
if ( href ) { | ||
$( this ).setAttribute( 'href', href.replace( '&classic-editor', '' ) ); | ||
} |
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.
Missed the jquery uses down here. Something like this should work.
$( 'body.revision-php a' ).each( function () { | |
var href = $( this ).getAttribute( 'href' ); | |
if ( href ) { | |
$( this ).setAttribute( 'href', href.replace( '&classic-editor', '' ) ); | |
} | |
for ( const node of document.querySelectorAll( 'body.revision-php a' ) ) { | |
const href = node.getAttribute( 'href' ); | |
if ( href ) { | |
node.setAttribute( 'href', href.replace( '&classic-editor', '' ) ); | |
} |
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 no longer worked anyway as we needed to check if the page had loaded properly first, so has been rewritten.
@@ -94,16 +92,12 @@ public static function load_features() { | |||
|
|||
// Initializers, if needed. | |||
\Marketplace_Products_Updater::init(); | |||
\Jetpack_Calypsoify::get_instance(); |
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.
Either bring back the use
and drop the \
, or do \Automattic\Jetpack\Calypsoify\Jetpack_Calypsoify
here.
} ); | ||
var editPostHeaderInception = setInterval( function () { | ||
var closeButton = document.querySelector( '.edit-post-fullscreen-mode-close__toolbar a' ); | ||
if ( closeButton ) { |
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.
if ( closeButton ) { | |
if ( ! closeButton ) { |
…or remove code where no longer needed
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.
On Simple:
- Same styles seem to be coming from 2 different places:
wp-content/admin-plugins/calypsoify/style-gutenberg.css?m=1676281014i&ver=0.2.1
wp-content/mu-plugins/jetpack-mu-wpcom-plugin/moon/vendor/automattic/jetpack-calypsoify/dist/index.css?m=1716281754i&minify=false&ver=b40fe4e9b3851a31e12d
- js is also duplicated:
Without being sandboxed: When appending &calypsoify=1
I still see the Fullscreen mode option:
- Script is loaded once, however it looks like styles are not loaded?
Is the desired behaviour for Simple sites the one we see without being sandboxed?
@fgiannar thanks for highlighting that! That is interesting - when Sandboxed the assets do load, even just on trunk with no additional changes. But when sandboxed, they don't. What I've noticed is that when sandboxed, admin-plugins is used, but it isn't in production. There is probably some documentation somewhere around that, I can look into. Also, the mu-wpcom changes are the only ones used on Simple sites (not the package required via Jetpack). As for the duplicated scripts, that was because of the package being loaded via mu-wpcom plugin as well as the admin-plugin. In short, this is what I think I should do:
|
I've edited the PR description with more info and removed Simple testing steps - I did add the CSS tweak directly to the admin-plugins version as well, in the linked diff. I realized that on Simple the module from Jetpack was not being loaded at all previously, so it left two options: Either remove There are also some differences between the two - JS relevant only to Simple in the admin-plugins version, and some minor PHP relevant to Simple too. I imagine it could probably be done (moving to mu-wpcom package version), but that would also require even more research to see what other side effects there may be. I think initializing the package version only in Atomic makes sense just now. |
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.
Great work, Karen! Many thanks!
Left a few inline comments. Other than that I tested the functionality on self-hosted, WoA and Simple and everything works as described 👍
// PhanTypeMismatchPropertyDefault : 1 occurrence | ||
|
||
// Currently, file_suppressions and directory_suppressions are the only supported suppressions | ||
'file_suppressions' => [ | ||
'src/class-jetpack-calypsoify.php' => ['PhanTypeMismatchArgumentProbablyReal', 'PhanTypeMismatchPropertyDefault'], | ||
'src/class-jetpack-calypsoify.php' => ['PhanTypeMismatchPropertyDefault'], |
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 believe we can get rid of this entire file by fixing the issue in class-jetpack-calypsoify.php
line 23 by replacing with @var object
with @var self|false
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.
Nice, thanks!
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 👍
…vent hiding additional menu item
.interface-more-menu-dropdown__content .components-dropdown-menu__menu:first-child .components-menu-group:first-child .components-button:last-of-type, | ||
.more-menu-dropdown__content .components-dropdown-menu__menu:first-child .components-menu-group:first-child .components-button:last-of-type { |
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 prevents an issue with other dropdown menu items being hidden when Calypsoified (for example 'Mobile' when clicking the Preview icon). See D149786-code for more context.
Two different selectors are used because .interface-more-menu-dropdown__content
is the correct classname in WordPress core (also used in Jetpack), but .more-menu-dropdown__content
is what is used on Atomic and Simple. Keeping the core version for additional safety.
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 the CSS hotfix 👍
Flagging this Slack thread - p1716540893951409/1716452544.422759-slack-C05PV073SG3 - some testing discrepancies noticed this morning related to the WordPress.com post editor URLs (WordPress.com vs wp-admin URLs from the WordPress.com posts page if this branch is checked out, vs not). It looks as though this was localized to existing test sites, not replicable elsewhere, but noting it here too. |
Fixes https://github.com/Automattic/vulcan/issues/320
Proposed changes:
mods-gutenberg.js
file there is loading correctly on the revisions page. Diff here: D149239-codecalypsoifyGutenberg
variable defined in the main php file includescloseUrl
,manageReusableBlocksUrl
andcreateNewPostUrl
.calypsoifyGutenberg
is still needed for the posts navigation on Atomic sites - this is thecloseUrl
in particular, and it is added to the sidebar via the ETK plugin. Removing it here does affect the relevant link on Atomic sites. The other two variables -manageReusableBlocksUrl
andcreateNewPostUrl'
- appear to be used in/widgets.wp.com/wpcom-block-editor/calypso.editor.js
. I'm not familiar with this and digging in would take considerably more time and is already out of the scope of this project, so for the sake of not breaking anything else I've left thecalypsoifyGutenberg
variable and associated functions intact.admin-plugins
version of Calypsoify? Some of the code is WPcom specific (some in the JS file in admin-plugins). There are also small differences in the main php file so workarounds would need to be added in the mu-wpcom package version if possible. In short we could, but for now I'm opting to keep things as is on Simple. Should there ever be more work done on Calypsoify in the future maybe it would be worthwhile. There are also potential unknowns at this stage and more testing needed if removing the admin-plugins version.Related diff: D149239-code
Other information:
Jetpack product discussion
https://github.com/Automattic/vulcan/issues/320
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Jetpack self hosted and Atomic:
jetpack build packages/calypsoify --production
, jetpack build packages/jetpack-mu-wpcom --productionand
jetpack build plugins/mu-wpcom-plugin --production`.calypsoify=1
param to the URL. Example:https://example.com/wp-admin/post.php?post=827&action=edit&calypsoify=1
.calypsoify_wpadminmods_js-css
andcalypsoify_wpadminmods_js-js-extra
).&calypsoify=1
from the URL, and refresh. Click on the 'options' menu, and you should see the option to toggle off Fullscreen mode. Click this, and refresh the page to confirm the change sticks./wp-admin/revision.php?revision=122&calypsoify=1
). The revision page should now be full-screen (vs. not without Calypsoification).Atomic-specific:
Simple site:
The changes added here should not affect Simple sites. The main thing to check is whether or not additional stylesheets or scripts are being loaded on Simple sites (using the steps to test this PR on Simple from the generated comment below) - checking the page source when Calypsoifying a (non-Classic-editor) post you shouldn't see duplications and assets should be coming from
admin-plugins
.Note that more realistic way to test Calypsoification is to visit
https://wordpress.com/block-editor/post/
and select a self hosted site.Fixes related to the revisions page and CSS on Simple (copying the changes made here which also is relevant on Simple) can be tested here: D149239-code