[RNMobile] Fix broken block registration #33890

SiobhyB · 2023-10-31T17:03:49Z

Fixes a regression in the Jetpack app that followed the changes in #32697. With this fix, it's possible to add Jetpack blocks within the apps again.

Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#6342

Proposed changes:

The registerJetpackBlockFromMetadata function has been taken from the web's register-jetpack-block.js file add added to register-jetpack-block.native.js for use in the apps.
Some tweaks have been made to ensure the function works as expected with the apps, including the use of a custom function (SvgXml from the react-native-svg library) for compatibility with React Native.

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

N/A

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

No, it does not.

Testing instructions:

With these changes checked out, navigate to the editor in the iOS or Android app.
Select + to insert a new block, verify that the following Jetpack blocks are visible:
- Contact Info
- Story
- Tiled Gallery (dev-environments only)
Verify that it's possible to add each block and that any edits are saved as expected.

Before	After

Following #32697, 'registerJetpackBlockFromMetadata' is now used to register some Jetpack blocks from metadata. As such, we need to support that functionality on native. With this commit, the function has been copied/pasted to the native file for use registering Jetpack blocks in the app.

github-actions · 2023-10-31T17:04:57Z

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 December 5, 2023 (scheduled code freeze on November 28, 2023).

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

github-actions · 2023-10-31T17:05:00Z

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 rnmobile/fix-broken-block-registration branch.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack rnmobile/fix-broken-block-registration

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

anomiex · 2023-11-01T21:08:56Z

Build is failing because react-native-svg does not resolve in the non-native build. You may need to add a .native.js copy of the file or something.

This reverts commit a7e593d.

With this PR, a version of the get-block-icon-from-metadata.js file has been added. This initial version is simply a copy/paste of the web file but will be iterated on for the native use-case. A separate native file is needed for a couple of reasons: 1. DOM parsing is unavailable with native, so the default web functionality doesn't work as expected in the app. 2. It's not possible to import React Native libraries to the main web files, as per the following comment: #33890 (comment)

As we don't have access to the DOM in React Native, we need to adjust this function. The `SvgXml` function that comes with `react-native-svg` has been used for this purpose.

anomiex · 2023-11-02T15:46:33Z

projects/js-packages/shared-extension-utils/src/get-block-icon-from-metadata.native.js

+ * @returns {JSX.Element} Icon component
+ */
+export function getBlockIconComponent( metadata ) {
+	return <SvgXml xml={ metadata.icon } />;


What happens if the block has no metadata.icon, or has it set to a non-svg like instagram-gallery currently does?

jetpack/projects/plugins/jetpack/extensions/blocks/instagram-gallery/block.json

Line 11 in 6960749

"icon": "instagram",

@anomiex, good point! @fluiddot has suggested another approach via Slack that I'm planning to follow instead of this. I'll work on this and set as ready for review again when it's ready. (Just juggling a few tasks today, sorry for not being super responsive!)

Noting that, to address this, in WordPress/gutenberg#55742, a change has been made directly to components/block-icon/index.native.js so that the parsing is handled on the Gutenberg side.

Following the discovery that the icon can be passed to components other than <BlockIcon/>, I reverted the aforementioned change, so we're back to parsing the icon in get-block-icon-from-metadata.native.js.

To account for cases where there may not be a block icon, or it may not be an svg string, I made some changes in 1a12767. I believe these changes bring us in alignment to how the logic works in the web file.

I used the instagram example to test that this works locally:

anomiex · 2023-11-02T15:48:07Z

You'll need to address the failing Linter test (running tools/fixup-project-versions.sh should make the necessary changes). All the E2E test failures look like known issues, you can ignore them.

I don't know enough about the apps to test or review that part of it myself though.

For native, we're updating the logic for parsing a block icon if it's a string. See: WordPress/gutenberg#55742 Given those changes, we can now pass the "metadata.icon" string directly when registering a block on native.

fluiddot

@SiobhyB I encountered an error when adding a Story block:

Seems we'd need to support SVG XML string sources directly in the Icon component, as the block icon might be used in more places apart from the BlockIcon component:

jetpack/projects/plugins/jetpack/extensions/blocks/story/edit.native.js

Line 25 in eebad7d

const icon = getBlockIconComponent( metadata );

jetpack/projects/plugins/jetpack/extensions/blocks/story/edit.native.js

Lines 175 to 176 in eebad7d

    
           <MediaPlaceholder 
        
           	icon={ icon }

projects/js-packages/shared-extension-utils/src/get-block-icon-from-metadata.js

projects/plugins/jetpack/extensions/shared/register-jetpack-block.native.js

This reverts commit 1fdc69a.

Following on from these comments, logic has been added to check whether an icon is present or not, and also allow it to not be a string: #33890 (comment)

SiobhyB · 2023-11-06T17:16:19Z

@fluiddot, thank you for the thorough review here!

Seems we'd need to support SVG XML string sources directly in the Icon component, as the block icon might be used in more places apart from the BlockIcon component:

I have a sense we should stick to parsing the icon in get-block-icon-from-metadata.native.js in the Jetpack codebase. The reason behind my thinking is that there could be a number of places where the icon could be passed, and the equivalent web components haven't been updated to account for this in Gutenberg. Do you think that seems sound? I've made some changes that appear to work for our use case, and also seem tweaks in 1a12767 to account for other potential issues mentioned in this review. Looking forward to hearing what you think.

projects/js-packages/shared-extension-utils/src/get-block-icon-from-metadata.native.js

As per the conversation here, we want to avoid importing SvgXml directly as it's not a dependency in the Jetpack codebase: #33890 (comment) As discussed, we'll instead import from Gutenberg directly.

fluiddot

LGTM 🎊 ! Thanks @SiobhyB for addressing this issue 🙇 !

I noticed that one of the E2E tests failed (Jetpack blocks e2e tests), the failure seems unrelated to these changes so I went ahead and restart it.

projects/js-packages/shared-extension-utils/src/get-block-icon-from-metadata.native.js

Co-authored-by: Carlos Garcia <fluiddot@gmail.com>

fluiddot

LGTM 🎊 !

Siobhan added 6 commits October 31, 2023 16:36

Expand on 'nameOrMetaData' code comment

a40e25b

Handle 'jetpack/' prefix

11bb40b

Fix typo in code comment

9521788

Parse block icon correctly on native

a7e593d

Update CHANGELOG entries

2dd8313

SiobhyB added [Type] Bug When a feature is broken and / or not performing as intended [Pri] High labels Oct 31, 2023

github-actions bot added [JS Package] Shared Extension Utils [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Status] In Progress RNA labels Oct 31, 2023

Ensure block title is passed from metadata

f043df7

SiobhyB mentioned this pull request Oct 31, 2023

Fix broken Jetpack block registration wordpress-mobile/gutenberg-mobile#6342

Merged

2 tasks

SiobhyB removed the [Status] In Progress label Oct 31, 2023

github-actions bot added the [Status] In Progress label Oct 31, 2023

SiobhyB requested review from monsieur-z and jeherve October 31, 2023 17:28

SiobhyB added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Oct 31, 2023

github-actions bot added the [Status] In Progress label Oct 31, 2023

SiobhyB marked this pull request as ready for review October 31, 2023 17:31

SiobhyB requested a review from fluiddot October 31, 2023 18:44

anomiex added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 1, 2023

Siobhan added 2 commits November 2, 2023 11:34

Merge branch 'trunk' into rnmobile/fix-broken-block-registration

d849221

Revert "Parse block icon correctly on native"

482e9a7

This reverts commit a7e593d.

Siobhan added 2 commits November 2, 2023 11:45

Adjust getBlockIconComponent for native use

6960749

As we don't have access to the DOM in React Native, we need to adjust this function. The `SvgXml` function that comes with `react-native-svg` has been used for this purpose.

anomiex reviewed Nov 2, 2023

View reviewed changes

SiobhyB marked this pull request as draft November 2, 2023 15:57

Siobhan added 3 commits November 3, 2023 08:39

Pass icon as string when registering native block

1fdc69a

For native, we're updating the logic for parsing a block icon if it's a string. See: WordPress/gutenberg#55742 Given those changes, we can now pass the "metadata.icon" string directly when registering a block on native.

Merge branch 'trunk' into rnmobile/fix-broken-block-registration

86933df

Update shared-extension-utils package name

eebad7d

SiobhyB marked this pull request as ready for review November 3, 2023 10:10

fluiddot reviewed Nov 3, 2023

View reviewed changes

Siobhan added 2 commits November 6, 2023 16:12

Revert "Pass icon as string when registering native block"

4a01eb1

This reverts commit 1fdc69a.

Account for when icon is not present or is non-svg

1a12767

Following on from these comments, logic has been added to check whether an icon is present or not, and also allow it to not be a string: #33890 (comment)

Merge branch 'trunk' into rnmobile/fix-broken-block-registration

30cad00

fluiddot reviewed Nov 6, 2023

View reviewed changes

projects/js-packages/shared-extension-utils/src/get-block-icon-from-metadata.native.js Outdated Show resolved Hide resolved

fluiddot mentioned this pull request Nov 7, 2023

Jetpack blocks registration: Use the web approach to identify beta version blocks wordpress-mobile/gutenberg-mobile#6357

Open

Siobhan added 2 commits November 7, 2023 10:12

Import SvgXml directly from Gutenberg

647795f

As per the conversation here, we want to avoid importing SvgXml directly as it's not a dependency in the Jetpack codebase: #33890 (comment) As discussed, we'll instead import from Gutenberg directly.

Merge branch 'trunk' into rnmobile/fix-broken-block-registration

c3a52c6

fluiddot previously approved these changes Nov 7, 2023

View reviewed changes

projects/js-packages/shared-extension-utils/src/get-block-icon-from-metadata.native.js Outdated Show resolved Hide resolved

Update JSDoc with up-to-date info around returned value

569649d

Co-authored-by: Carlos Garcia <fluiddot@gmail.com>

SiobhyB dismissed fluiddot’s stale review via 569649d November 7, 2023 14:25

fluiddot approved these changes Nov 7, 2023

View reviewed changes

SiobhyB merged commit 3c38ce3 into trunk Nov 7, 2023
64 of 65 checks passed

SiobhyB deleted the rnmobile/fix-broken-block-registration branch November 7, 2023 14:51

github-actions bot removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] In Progress labels Nov 7, 2023

github-actions bot added this to the jetpack/12.9 milestone Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Fix broken block registration #33890

[RNMobile] Fix broken block registration #33890

SiobhyB commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

anomiex commented Nov 1, 2023

anomiex Nov 2, 2023

SiobhyB Nov 2, 2023

SiobhyB Nov 3, 2023

SiobhyB Nov 6, 2023

anomiex commented Nov 2, 2023

fluiddot left a comment

SiobhyB commented Nov 6, 2023

fluiddot left a comment

fluiddot left a comment

[RNMobile] Fix broken block registration #33890

[RNMobile] Fix broken block registration #33890

Conversation

SiobhyB commented Oct 31, 2023 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

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

Testing instructions:

github-actions bot commented Oct 31, 2023 • edited Loading

github-actions bot commented Oct 31, 2023 • edited Loading

anomiex commented Nov 1, 2023

anomiex Nov 2, 2023

Choose a reason for hiding this comment

SiobhyB Nov 2, 2023

Choose a reason for hiding this comment

SiobhyB Nov 3, 2023

Choose a reason for hiding this comment

SiobhyB Nov 6, 2023

Choose a reason for hiding this comment

anomiex commented Nov 2, 2023

fluiddot left a comment

Choose a reason for hiding this comment

SiobhyB commented Nov 6, 2023

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot left a comment

Choose a reason for hiding this comment

SiobhyB commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading