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.

Sign up for GitHub

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

Jump to bottom

[RNMobile] Add WP hook for registering non-core blocks #52791

Merged
merged 3 commits into from
Jul 20, 2023
Merged

[RNMobile] Add WP hook for registering non-core blocks #52791

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e04b10
Add WP hook action for registering non-core blocks
fluiddot Jul 20, 2023
2edd4a8
Remove unused `native.render` WP hook
fluiddot Jul 20, 2023
7fb8ba6
Add test case to cover WP hook `post-register-core-blocks`
fluiddot Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/react-native-editor/src/setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { I18nManager, LogBox } from 'react-native';
* WordPress dependencies
*/
import { unregisterBlockType, getBlockType } from '@wordpress/blocks';
import { addAction, addFilter } from '@wordpress/hooks';
import { addAction, addFilter, doAction } from '@wordpress/hooks';
import * as wpData from '@wordpress/data';
import { registerCoreBlocks } from '@wordpress/block-library';
// eslint-disable-next-line no-restricted-imports
Expand Down Expand Up @@ -70,6 +70,8 @@ const setupInitHooks = () => {
) {
unregisterBlockType( 'core/block' );
}

doAction( 'native.post-register-core-blocks', props );
Copy link
Member

Choose a reason for hiding this comment

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

I've double-checked that the props passed here are the ones expected as from the older approach in https://github.com/WordPress/gutenberg/blob/trunk/packages/react-native-editor/src/index.js#L57-L66 👍

I was wondering if we should remove the native.render action or keep it if it's needed in the future, what do you think? I'm fine with keeping it by the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've double-checked that the props passed here are the ones expected as from the older approach in https://github.com/WordPress/gutenberg/blob/trunk/packages/react-native-editor/src/index.js#L57-L66 👍

Thanks for checking it 🙇 ! I saw that we only use the capabilities prop when registering the Jetpack blocks, which is already being passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering if we should remove the native.render action or keep it if it's needed in the future, what do you think? I'm fine with keeping it by the way.

Good question. AFAIK it's not being used anywhere, so I understand we could remove it. If we need it in the future we can always revert it back.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me! Are we planning to do that in a follow-up PR? This PR is ready for my approval so let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry. I'm planning to apply the suggestion in this PR. In fact, I noticed that I missed adding a unit test for the new hook. I'll let you know when I pushed the changes, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Cool! I'll keep an eye out for those, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geriux I've applied you suggestion 2edd4a8.

I also updated the unit tests in 7fb8ba6. It took way longer than I expected because the test logic required some tricky changes to check the call orders. Let me know if you have any concerns/questions regarding that, thanks 🙇 !

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I'll check it out

} );

// Map native props to Editor props
Expand Down
Loading