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

Classic Theme Helper: Require Social Menus from the package #38297

Merged

Conversation

coder-karen
Copy link
Contributor

@coder-karen coder-karen commented Jul 11, 2024

Fixes https://github.com/Automattic/vulcan/issues/394
Fixes https://github.com/Automattic/vulcan/issues/403

Note - this PR needs to be merged with timing that allows for a wpcomsh release to occur before the Jetpack release for WoA, otherwise the social menu items will not display until the next wpcomsh release.

Proposed changes:

  • This PR ensures we are requiring the newly moved Social Menu code from the Classic Theme Helper package, in the Jetpack and Jetpack Mu WPcom plugins.
  • A new file has been created in the Classic Theme Helper package - shared-functions.php - which now includes jetpack_mastodon_get_instance_list. This is copied from the Jetpack globals file, and is used by Social Menu and Social Links. Once Social Links are moved to the Classic Theme Helper package this can then be removed from the Jetpack plugin.
  • In jetpack/modules/widgets/social-icons.php, a call to the Social Menu SVG file has been replaced with a call to the package version, if it exists.

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/394

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Self-hosted:

  • Check this branch out locally (if doing so make sure to jetpack build plugins/jetpack and jetpack build packages/classic-theme-helper), or via the Jetpack Beta tester plugin.
  • Switch to a theme that already has support for jetpack-social-menu, Dara is a good example. Otherwise you can follow the steps in this PR to add support to an older theme: Social Menu & Social Media Icons: Add support for SMS. #36176
  • Go to Appearance > Customize > Menus
  • Create a new menu with some links, e.g. https://www.facebook.com/jetpackme/, https://x.com/jetpack and sms:0102030405
  • Assign that menu to the Social Menu location
  • Ensure the menu displays correctly
  • Ensure there are no deprecation notices or errors in the error logs
  • To test the moved function jetpack_mastodon_get_instance_list is being called from the package and not the Jetpack plugin, add the following snippet via a Code Snippets plugin (borrowed from this PR: Social Menu / Social Icons Widget: add Mastodon #28175):
// Add an extra instance to the list.
add_filter(
	'jetpack_mastodon_instance_list',
	function ( $mastodon_instance_list ) {
		$mastodon_instance_list[] = 'religious.social';
		return $mastodon_instance_list;
	}
);
  • In a code editor, comment out the Jetpack plugins version of this function - jetpack_mastodon_get_instance_list - which is in functions.global.php.
  • Ensure the icons continue to load as expected.
  • To test that the SVG icon file is being picked up from the package and not the Jetpack plugin, comment out the SVG file contents (social-menu/social-menu.svg) in both Jetpack and the package, and notice it won't work.
  • Comment out only the SVG file in the module file and it will work.
  • Comment out only the SVG file in the package and it won't.

WoA:

  • Test as with self-hosted, making sure to check out the branch in both Jetpack and WordPress.com Features (if using the Beta Tester plugin), and making sure your WoA site is set up for jetpack-mu-wpcom plugin testing with the snippet mentioned in the generated comment below.
  • When testing the SVG icon, use the Classic Theme Helper package version of the SVG file via the jetpack-mu-wpcom plugin (jetpack-mu-wpcom-plugin-dev if using the Beta tester plugin) - vendor/automattic/jetpack-classic-theme-helper/src/social-menu/social-menu.svg). This also has the benefit of confirming that we are loading via the package via the Jetpack Mu WPcom plugin as well.
  • Lastly, test that the menu items continue to display as expected with no errors in error logs if the PR is only applied to the WordPress.com Features plugin (via the Beta tester plugin), and that Jetpack is on the current stable version. This will simulate wpcomsh being updated and therefore jetpack-mu-wpcom plugin, before the Jetpack plugin updates occur.

Simple:

  • Follow the steps in the generated comment below to test the changes in both branches on a sandboxed Simple site.
  • Test as with the self-hosted steps to confirm that the menu displays as it should.
  • With a Simple site I'm not too sure how we'd go about testing the source of the jetpack_mastodon_get_instance_list function nor which version of the SVG file is used, but what happens on WoA should be what would happen on Simple.
    • Also, when testing the menu display on the front-end, you can open up the browser console, check the Sources tab, and make sure it is set to Page (not Workspace). Under wp-content/mu-plugins/jetpack-mu-wpcom-plugin you should notice that src/social-menu.css is loading via vendor/automattic/jetpack-classic-theme-helper'. Note as well that the social-menu module is not showing under the sources for the Jetpack plugin here either. Testing without the branches applied should show the opposite - the social-menu file is loaded via the Jetpack plugin, and not via jetpack-mu-wpcom-plugin`.
  • As with WoA, test with only the jetpack-mu-wpcom plugin branch changes applied as well, to simulare the period after a wpcomsh release and before Jetpack is updated. There should be no change in menu display, nor any errors in logs.
Copy link
Contributor

github-actions bot commented Jul 11, 2024

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 August 6, 2024 (scheduled code freeze on August 5, 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: WordPress.com Simple releases happen daily.

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


Wpcomsh plugin:

  • Next scheduled release: on demand (usually Mondays if not sooner).

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


Classic Theme helper plugin plugin:

  • Next scheduled release: none scheduled.

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 Jul 11, 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 add/require-social-menu-classic-theme-helper-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 add/require-social-menu-classic-theme-helper-package
    
    bin/jetpack-downloader test jetpack-mu-wpcom-plugin add/require-social-menu-classic-theme-helper-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
@coder-karen coder-karen marked this pull request as ready for review July 11, 2024 13:56
@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 Jul 11, 2024
@coder-karen coder-karen requested a review from a team July 11, 2024 13:57
@monsieur-z
Copy link
Contributor

I haven't had time to test simple sites yet, but everything worked as expected with self-hosted and WoA sites. Code changes seem sensible.

@coder-karen coder-karen removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jul 16, 2024
@coder-karen
Copy link
Contributor Author

Marking as 'on hold' until after the next WoA release after some testing discrepancies mentioned in Slack by @monsieur-z

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jul 18, 2024
@coder-karen coder-karen added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jul 18, 2024
@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 Jul 18, 2024
monsieur-z
monsieur-z previously approved these changes Jul 19, 2024
Copy link
Contributor

@monsieur-z monsieur-z left a comment

Choose a reason for hiding this comment

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

I've been able to confirm the code and icons are loaded from the package with simple sites too. Looking at the Source panel confirmed it as well.

I haven't seen any errors by applying this branch to just the package.

monsieur-z
monsieur-z previously approved these changes Jul 23, 2024
@coder-karen coder-karen merged commit 4bbcab7 into trunk Jul 24, 2024
56 checks passed
@coder-karen coder-karen deleted the add/require-social-menu-classic-theme-helper-package branch July 24, 2024 12:56
@github-actions github-actions bot removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment