Skip to content
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

Merged
merged 33 commits into from
May 24, 2024

Conversation

coder-karen
Copy link
Contributor

@coder-karen coder-karen commented May 14, 2024

Fixes https://github.com/Automattic/vulcan/issues/320

Proposed changes:

  • This PR requires the newly created Calypsoify package in both the Jetpack plugin and the mu-wpcom plugin (but makes sure the class initialization in the mu-wpcom package only happens if a site is Atomic, as that mirrors the current behaviour on Simple sites (the Jetpack module assets are not loaded there)).
  • It also modifies the webpack.config.js file (simplified - JS dealt with by the main Jetpack config and in terms of CSS it's just compiling CSS from SCSS), imports the scss file in the js file (in 'src'), and changes how assets are registered by using the Assets package.
  • There have been some CSS and JS tweaks to fix things that have broken at some stage in the past - predominantly the option to toggle off the full-screen editor is now hidden if Calypsoify is enabled.
  • What has been fixed:
    • Styles related to hiding the full-screen toggle option (via Options menu in post editor) if Calypsoified
  • What has been removed:
    • Styles to hide full-screen option from site editor view modes (option no longer exists)
    • Script to force full-screen for iframed site editor (can no longer be anything but that)
    • The scripts relevant to the close button have been removed as it's only relevant for Jetpack sites, and we're removing Calypsoify from self-hosted Jetpack (Atomic sites have a nav-bar in place thanks to the ETK plugin, which overrides that button).
    • The scripts relevant to the revision.php page have been removed as that was only relevant for Simple sites. Instead a change was made directly to admin-plugins to make sure the mods-gutenberg.js file there is loading correctly on the revisions page. Diff here: D149239-code
  • Additional info:
    • The calypsoifyGutenberg variable defined in the main php file includes closeUrl, manageReusableBlocksUrl and createNewPostUrl. calypsoifyGutenberg is still needed for the posts navigation on Atomic sites - this is the closeUrl 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 and createNewPostUrl' - 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 the calypsoifyGutenberg variable and associated functions intact.
    • Why don't we get rid of the 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:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

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:

  • To test changes on a self hosted site and Atomic, apply this PR.
  • If applying the PR locally, make sure to build the assets with jetpack build packages/calypsoify --production, jetpack build packages/jetpack-mu-wpcom --productionandjetpack build plugins/mu-wpcom-plugin --production`.
  • If using the Jetpack Beta plugin to test, follow the steps in the generated comment below, remember to apply the PR both for the Jetpack plugin and the 'WordPress.com Features' plugin via the Beta tester UI. More info here too on WoA testing: PCYsg-Osp-p2
  • Visit the wp-admin post editor for a post (not using the Classic Editor), and add the calypsoify=1 param to the URL. Example: https://example.com/wp-admin/post.php?post=827&action=edit&calypsoify=1.
  • Confirm the Calypso stylesheet / scripts are loaded when the query param is added by checking the page source (see calypsoify_wpadminmods_js-css and calypsoify_wpadminmods_js-js-extra).
  • Click on the 'options' menu, confirm that you see no option to toggle Fullscreen mode off.
  • Go back to the post and remove the &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.
  • Then, Calypsoify the URL and refresh the page again - the editor should now be full-screen, and the option to toggle off full-screen gone from the options menu.
  • Open up a revision for the post, and Calypsoify the URL ( for example /wp-admin/revision.php?revision=122&calypsoify=1). The revision page should now be full-screen (vs. not without Calypsoification).

Atomic-specific:

  • Additionally, on an Atomic test site, Calypsoify a post URL and then click on the site icon (top left). A sidebar should display (this is thanks to the ETK plugin). Hover over 'All posts' and the URL should the WordPress.com posts URL.
  • Do the same but without a Calypsoified post - the URL should be the wp-admin posts URL.

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

@coder-karen coder-karen self-assigned this May 14, 2024
@github-actions github-actions bot added [Feature] Calypsoify [Package] Calypsoify [Package] Jetpack mu wpcom WordPress.com Features [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] mu wpcom jetpack-mu-wpcom plugin labels May 14, 2024
Copy link
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.


Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

  • WordPress.com Simple releases happen daily.
  • WoA releases happen weekly.
  • Releases to self-hosted sites happen monthly. The next release is scheduled for June 4, 2024 (scheduled code freeze on June 3, 2024).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.


Mu Wpcom plugin:

  • Next scheduled release: June 4, 2024.
  • Scheduled code freeze: May 27, 2024.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Copy link
Contributor

github-actions bot commented May 14, 2024

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the update/calypsoify-load-package branch.

    • For jetpack-mu-wpcom changes, also add define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true ); to your wp-config.php file.
  • To test on Simple, run the following command on your sandbox:

    bin/jetpack-downloader test jetpack update/calypsoify-load-package
    
    bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/calypsoify-load-package
    

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

'../dist/index.js',
__FILE__,
array(
'jquery',
Copy link
Contributor

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

Suggested change
'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';.

Copy link
Contributor Author

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' ) ) {
Copy link
Contributor

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.

Suggested change
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?

Copy link
Contributor Author

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.

projects/packages/calypsoify/src/js/mods-gutenberg.js Outdated Show resolved Hide resolved
Comment on lines 35 to 39
$( 'body.revision-php a' ).each( function () {
var href = $( this ).getAttribute( 'href' );
if ( href ) {
$( this ).setAttribute( 'href', href.replace( '&classic-editor', '' ) );
}
Copy link
Contributor

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.

Suggested change
$( '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', '' ) );
}

Copy link
Contributor Author

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();
Copy link
Contributor

@anomiex anomiex May 16, 2024

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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( closeButton ) {
if ( ! closeButton ) {

Copy link
Contributor

@fgiannar fgiannar left a 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:
  1. wp-content/admin-plugins/calypsoify/style-gutenberg.css?m=1676281014i&ver=0.2.1
  2. 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:
Screenshot 2024-05-21 at 14 02 15

Without being sandboxed: When appending &calypsoify=1 I still see the Fullscreen mode option:

Screenshot 2024-05-21 at 14 45 25
  • Script is loaded once, however it looks like styles are not loaded?
Screenshot 2024-05-21 at 14 39 06

Is the desired behaviour for Simple sites the one we see without being sandboxed?

@coder-karen
Copy link
Contributor Author

@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:

  • Make any CSS tweaks that affect Simple directly in admin-plugins.
  • Make sure the mu-plugins package version is not loaded on Simple.

@coder-karen coder-karen added [Status] In Progress and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 21, 2024
@coder-karen
Copy link
Contributor Author

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 admin-plugins/calypsoify, or ensure the package version is only loaded on Atomic sites. To keep things more in parity with how they were I've opted for leaving the admin-plugins version and made sure the mu-wpcom package only initialized the Calypsoify class if a site is Atomic.

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.

@coder-karen coder-karen added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels May 22, 2024
Copy link
Contributor

@fgiannar fgiannar left a 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'],
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

fgiannar
fgiannar previously approved these changes May 23, 2024
Copy link
Contributor

@fgiannar fgiannar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Comment on lines +8 to +9
.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 {
Copy link
Contributor Author

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.

@coder-karen coder-karen requested a review from fgiannar May 24, 2024 07:39
Copy link
Contributor

@fgiannar fgiannar left a 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 👍

@fgiannar fgiannar added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 24, 2024
@coder-karen
Copy link
Contributor Author

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.

@coder-karen coder-karen merged commit 88511a9 into trunk May 24, 2024
73 checks passed
@coder-karen coder-karen deleted the update/calypsoify-load-package branch May 24, 2024 12:58
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Calypsoify [Package] Calypsoify [Package] Jetpack mu wpcom WordPress.com Features [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] mu wpcom jetpack-mu-wpcom plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants