jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

Classic Theme Helper: Requiring the Responsive Video package files, and modifying Featured Content require process

Open coder-karen opened this issue 1 month ago • 7 comments

Fixes https://github.com/Automattic/vulcan/issues/345 Fixes https://github.com/Automattic/vulcan/issues/346

Proposed changes:

  • The intention of this PR was to require Responsive Videos from the Classic Theme Helper package in Jetpack, the Classic Theme helper plugin, and the Jetpack Mu WPcom plugin. In the process changes were made which affected how Featured Content files were required, so those changes will also need testing here.
  • Specifically, this PR updates the Classic Theme Helper package's Main class to require both the responsive-videos.php file and the featured-content.php file (removing the plugins_loaded action as this wasn't running previously).
  • The Featured_Content setup function is called directly from the Featured Content file within the Classic Theme Helper package. ~Now that file is being required, the setup function doesn't need to be called independently - except for in the jetpack-mu-wpcom package, specifically for Simple sites who do not pick this change up otherwise.~ The Featured_Content setup function is also called directly anywhere where it is needed (the jetpack-mu-wpcom package, ad currently the Jetpack plugin though that will be removed after a short period of time)

What could be changed:

  • ~Any Classic Helper Theme file that is a class could be called independently (not included in the package's Main class init function)~ Based on the review, we'll follow that (hence calling the Featured_Content setup function wherever that class is needed, instead of requiring the file).

Noting that we're not loading theme tools compat files from the package just yet as there is other functionality included in the compat files, so seems best to wait until all else is moved. Also we should be able to delete the mu-plugins version here: wp-content/mu-plugins/responsive-videos.php - or require the package version - but will wait until of these changes have made their way to WPcom and follow up then.

Other information:

  • [ ] Have you written new tests for your changes, if applicable?
  • [x] Have you checked the E2E test CI results, and verified that your changes do not break them?
  • [x] 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/345

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

No.

Testing instructions:

Self-hosted / Atomic:

To test existing behaviour:

  • For responsive videos, first ensure you are using a theme that has added support for responsive videos (for example Twenty Fifteen or Twenty Nineteen).
  • Then, create a post or page and embed a video. I found that the best way to ensure I was using responsive videos was to use the VideoPress block (which requires enabling VideoPress at Settings > Performance > Media: /wp-admin/admin.php?page=jetpack#/performance, so you'll need a WordPress.com connected site to test that)
  • View the post / page source for the published post / page. Notice the jetpack-responsive-videos-js script in the page source (and that it's source is from the jetpack plugin (or jetpack-dev if using the beta tester plugin) ), and that the video is wrapped in a div with the class jetpack-video-wrapper.
  • For Featured Content, with a theme that supports Featured Content (Dara is an example), open the Customizer. You should see a 'Featured Content' section. When opening the page source from here, you should see the featured-content-suggest-js (and that it's source on Atomic is from the jetpack-mu-wpcom-plugin plugin (or jetpack-mu-wpcom-plugin-dev if using the beta tester plugin), or jetpack / jetpack-dev on self-hosted sites ).
  • Confirm everything works as it should: Add a tag and click save, then apply that tag to a few posts - the featured content should display on the homepage.

To test this PR:

  • Apply the PR. If on a JN site make sure to apply the PR for both Jetpack and WordPress.com Features plugins. If building locally, run jetpack build plugins/jetpack, jetpack build packages/jetpack-mu-wpcom, and jetpack build plugins/mu-wpcom-plugin
  • For responsive videos, follow the steps above, then view the post / page source for the published post / page. Notice the jetpack-responsive-videos-js script in the page source (and that it's source is from the jetpack-mu-wpcom-plugin on Atomic and jetpack on self-hosted sites ) , and that the video is wrapped in a div with the class jetpack-video-wrapper.
  • For the Featured Content changes in this PR, follow the same testing steps as when testing existing behaviour.
  • Everything should test as it did before (and the featured-content-suggest-js script source is the same as before as well).

On Simple:

To test existing behaviour:

  • For responsive videos, follow the same steps as with self hosted sites (VideoPress should be enabled by default, if using the VideoPress block).
  • Notice the responsive-videos.min.js script is showing in page source, via the jetpack plugin, and that the video is wrapped in a div with the class jetpack-video-wrapper.
  • Featured Content can be tested as with self-hosted / Atomic.

To test this PR:

  • Follow the steps to apply the changes on a Simple site from the generated comment below.
  • Follow the same steps as above - notice the jetpack-responsive-videos-js script shows in page source with the jetpack-mu-wpcom-plugin as it's source (and that the video is wrapped in a div with the class jetpack-video-wrapper).
  • For Featured Content, follow the same steps as with self-hosted / Atomic. Everything should test as it did before (and the featured-content-suggest-js script source is the same as before as well).

Note that for both Simple and Atomic / Self-hosted, you can confirm that we are not relying on the Jetpack plugin to initialize the classic-theme-helper packages by modifying code directly (comment out 'theme-tools/featured-content.php', and 'theme-tools/responsive-videos.php', in projects/plugins/jetpack/modules/module-extras.php).

Noting that I've removed testing instructions for the Classic Theme Helper plugin as it looks like more work may now be needed: https://github.com/Automattic/vulcan/issues/384

coder-karen avatar Jun 20 '24 16:06 coder-karen