Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Try moving blocks to their own modular directories #170

Merged
merged 3 commits into from
May 16, 2023

Conversation

johnstonphilip
Copy link
Contributor

@johnstonphilip johnstonphilip commented May 15, 2023

Tasks

Summary of changes

This PR refactors the Pattern Block's (aka Patternception's) code into its own dedicated directory.

How to test

  1. Just make sure the Pattern Blocks works as it should. No changes should actually happen with this PR.

Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Hi @johnstonphilip,
Good idea to move block functions to their own directory.

The Pattern Block works the same as before.

'blocks.registerBlockType',
'pattern-manager/registerPatternBlock',
registerPatternBlock
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving addFilter() out of this file is the only part of this PR I don't like.

The idea of this file was to run all of the side-effects of the editor/ module.

So importing another file shouldn't run side-effects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, it's just my opinion 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I can see the argument for both ways. My preference is to be able to remove the whole block by simply deleting a directory. But I'd be alright with putting it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, it's a minor issue 😄 Whatever you'd like.

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 moved it back :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot!

'blocks.registerBlockType',
'pattern-manager/registerPatternBlock',
registerPatternBlock
);

export default function registerPatternBlock(
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to move the block-related functions to blocks/

return () => ( {} );
} );

jest.mock( '../../../../../app/js/src/components/PatternPreview', () => {
jest.mock( '../../../../../../app/js/src/components/PatternPreview', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mocking for React components is so messy 🤣

@johnstonphilip johnstonphilip marked this pull request as ready for review May 15, 2023 19:38
Copy link
Contributor

@mike-day mike-day left a comment

Choose a reason for hiding this comment

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

This looks good, @johnstonphilip!

The structure is a bit easier to follow this way — it's nice cleanup.

@johnstonphilip johnstonphilip merged commit 141fff9 into main May 16, 2023
@johnstonphilip johnstonphilip deleted the try/move-blocks-to-their-own-files branch May 16, 2023 14:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants