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

MU WPCOM: Port the newspack blocks from the ETK (2nd try) #38724

Merged
merged 31 commits into from
Aug 7, 2024

Conversation

arthur791004
Copy link
Contributor

@arthur791004 arthur791004 commented Aug 6, 2024

Related to p1722871269505439-slack-CRWCHQGUB

Proposed changes:

  • We merged MU WPCOM: Port the newspack blocks from the ETK #38454 before but it had a conflict with the newspack-blocks plugin on atomic sites, so the changes were reverted yesterday.
  • Centralize the feature flags of the ETK plugin by the load_etk_features_flags function & Load the newspack blocks feature only when the Newspack_Blocks isn't declared- 2803837
  • Load the ETK features as normal but limit the editor-related features to the editor pages or frontend pages to avoid fatal errors when activating the plugin - 6039568

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

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

Testing instructions:

  • Go to your atomic site
  • Install the newspack-blocks plugin via https://github.com/Automattic/newspack-blocks/releases/tag/v3.6.0
  • Make sure the fatal error won't be shown
    PHP Fatal error:  Cannot declare class Newspack_Blocks, because the name is already in use in /wordpress/plugins/wpcomsh/5.2.0/vendor/automattic/jetpack-mu-wpcom/src/features/newspack-blocks/synced-newspack-blocks/class-newspack-blocks.php on line 11
    

Copy link
Contributor

@taipeicoder taipeicoder left a comment

Choose a reason for hiding this comment

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

The changes LGTM, see the conversation in p1722993219555279-slack-CRWCHQGUB for a more in-depth review.

@arthur791004 arthur791004 merged commit d49e2d3 into trunk Aug 7, 2024
74 checks passed
@arthur791004 arthur791004 deleted the mu-wpcom/newspack-blocks branch August 7, 2024 06:07
);

add_action(
is_admin() ? 'enqueue_block_assets' : 'enqueue_block_editor_assets',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add these conditions? It should always be enqueue_block_editor_assets for block scripts that register the blocks. See #41055.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment