-
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
Classic Theme Helper: Require Social Menus from the package #38297
Classic Theme Helper: Require Social Menus from the package #38297
Conversation
… for class in Jetpack module file.
…ge where it will be needed
…he package exists, from the social-icons file in Jetpack.
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. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Classic Theme helper plugin 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?
|
…n as function in question is behind a function_exists check.
…enu-classic-theme-helper-package
…_instance_list, to be safe
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. |
Marking as 'on hold' until after the next WoA release after some testing discrepancies mentioned in Slack by @monsieur-z |
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'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.
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 nextwpcomsh
release.Proposed changes:
shared-functions.php
- which now includesjetpack_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.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:
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:
jetpack build plugins/jetpack
andjetpack build packages/classic-theme-helper
), or via the Jetpack Beta tester plugin.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. #36176https://www.facebook.com/jetpackme/
,https://x.com/jetpack
and sms:0102030405jetpack_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):jetpack_mastodon_get_instance_list
- which is infunctions.global.php
.social-menu/social-menu.svg
) in both Jetpack and the package, and notice it won't work.WoA:
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.wpcomsh
being updated and thereforejetpack-mu-wpcom
plugin, before the Jetpack plugin updates occur.Simple:
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.wp-content/mu-plugins/jetpack-mu-wpcom-plugin
you should notice thatsrc/social-menu.css
is loading viavendor/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`.jetpack-mu-wpcom
plugin branch changes applied as well, to simulare the period after awpcomsh
release and before Jetpack is updated. There should be no change in menu display, nor any errors in logs.